Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated save messaging and added the storyline title + scrollbar to UUID dropdown #316

Merged
merged 1 commit into from
Jun 14, 2024

Conversation

szczz
Copy link
Member

@szczz szczz commented Jun 7, 2024

Updated save messaging / behaviour and added the storyline title + scrollbar to UUID dropdown

Related Item(s)

#301

Changes

  • [FEATURE] Introduces a scrollbar and the title at the time of saving to the dropdown. Need to discuss bi-lingual title storage in the next meeting.

  • [REFACTOR] Refactored the versioning system. Mostly done in the .NET API, but I added a warning and disabled the save button until the save is fully complete.

Notes

Can be tested on dev. I used the DevDeployment branch to release this code.

Testing

Steps:

  1. Open dev
  2. Verify the UUID dropdown doesn't scroll off the page.
  3. Use the vertical scroll bar to your enjoyment
  4. Verify the title for the Storyline is there. (This will be the title at the time of creation. Need to discuss). *** Only updated the title for a few of the storylines manually.
  5. Try to save a storyline 5+ times. Confirm the previous versions load ALL GOOD. After the 6th save the oldest storyline should be nuked.

This change is Reviewable

Copy link

github-actions bot commented Jun 7, 2024

Your demo site is ready! 🚀 Visit it here: https://ramp4-pcar4.github.io/storylines-editor/QOLChanges

Copy link
Member

@yileifeng yileifeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The title appended to dropdown list items is a nice enhancement. Two notes (one minor one major):

  • the current dropdown is not keyboard accessible
  • is it worth bolding one of the UUID or title text?

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @szczz)


src/components/metadata-editor.vue line 61 at r1 (raw file):

                            />
                            <div
                                class="absolute z-10 w-full bg-white border border-gray-200 mt-1 max-h-96 overflow-y-auto"

Would it be better to make the max-height relative to screen size (e.g. 60vh)?

… disabled the button until complete. Added the title and a scrollbar to UUID dropdown
Copy link
Member Author

@szczz szczz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

donethanks

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @yileifeng)


src/components/metadata-editor.vue line 61 at r1 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

Would it be better to make the max-height relative to screen size (e.g. 60vh)?

donethanks

Copy link
Member

@RyanCoulsonCA RyanCoulsonCA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yileifeng)

Copy link
Member

@yileifeng yileifeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @szczz)

@yileifeng yileifeng merged commit 0be9c27 into ramp4-pcar4:main Jun 14, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants