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

Implement minor slide editor redesign, improve 'full slide' functionality #486

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

gordlin
Copy link
Member

@gordlin gordlin commented Dec 9, 2024

Related Item(s)

Issues:
#450
#453
#457

Changes

  • Implements a minor redesign of the slide editor.
    • Adjusts the header/common section to enhance clarity.
    • Round-ifys buttons and inputs so they match the ToC and metadata editor.
    • Unifies the outlines to a common 'royal blue' colour upon keyboard focus, to make it more clear what's been selected.
    • Moves most input labels to be a line before the input, which prevents overlap on small screens and matches design on metadata editor.
    • Adds a very minor shadow to inputs to emphasize their existence.
    • Updates the warning modals for Change content type and Make the current panel the full slide to use the new, clearer design.
    • Adjusts the styles of the text and image editors to improve clarity. (NOTE: remaining editors haven't really been modified; could be a TODO for the future).
      • Changes breakpoints for the image previews so they don't get too small to see (largest screen allows 3 images in a row, medium screens allow 2, mobile-like screens allow 1).
  • [The "Make the right panel the full slide" feature #450] Changes the Make right panel the full slide option to Make the current panel the full slide.
    • Upon disabling the option, user will now be asked which side they want the new panel to be on (left or right) (See image below).
  • [Vertical scrollbar appears when no slide is selected #457] Prevents vertical scrollbar from appearing when editor page first loaded and no slide is selected.

Notes

New common slide editor header:
image

Enabling Make the current panel the full slide modal:
image

Disabling Make the current panel the full slide modal:
image

Change panel type modal:
image

Image editor (large screens):
image

Image editor (medium screens):
image

Image editor (small screens):
image

Testing

Steps:

  1. Open any product (e.g. 00000000-0000-0000-0000-000000000000). Immediately notice there's no vertical scroll bar for the slide editor area.
  2. Open any slide. See the modified common slide elements (title, slide options, etc.) at the top.
  3. Test out the Make current panel the full slide option. Select it, select continue for the warning, and the panel will be the whole slide. De-select the option, and you can choose which side to put the new panel on.

This change is Reviewable

@gordlin gordlin added PR: Active PRs that require a fierce eyeballin. PR: Frontend PR that primarily involves frontend changes. UI experts and CSS Wizards are asked to review. labels Dec 9, 2024
Copy link

github-actions bot commented Dec 9, 2024

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

@gordlin gordlin force-pushed the editor-issues branch 6 times, most recently from bbc2d8e to 9725716 Compare December 12, 2024 20:55
Copy link
Member

@IshavSohal IshavSohal left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 14 files reviewed, 1 unresolved discussion (waiting on @gordlin)


a discussion (no related file):
Looks great, everything seems to be working as intended. However I noticed that for slideshow panels, the styling for the captions got a bit messed up:

image.png

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 very nice, good work!

Reviewed 14 of 14 files at r1.
Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @gordlin)


src/components/helpers/multi-option-modal.vue line 27 at r1 (raw file):

                    {{ $t('editor.cancel') }}
                </button>
                <!--                <button class="editor-button bg-black text-white hover:bg-gray-800" @click="onOk">-->

These comments can probably be removed before we merge.

Copy link
Member

@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.

Just a heads up that there are a few merge conflicts

Reviewed 14 of 14 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gordlin)

@gordlin gordlin force-pushed the editor-issues branch 2 times, most recently from c54fe3d to 59c120f Compare December 18, 2024 21:09
Copy link
Member Author

@gordlin gordlin left a comment

Choose a reason for hiding this comment

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

Reviewable status: 8 of 14 files reviewed, 2 unresolved discussions (waiting on @IshavSohal, @RyanCoulsonCA, and @szczz)


a discussion (no related file):

Previously, IshavSohal (Ishav Sohal) wrote…

Looks great, everything seems to be working as intended. However I noticed that for slideshow panels, the styling for the captions got a bit messed up:

image.png

Donethanks! Also moved the caption input to the top (above the image preview stuff) for easier access.


src/components/helpers/multi-option-modal.vue line 27 at r1 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

These comments can probably be removed before we merge.

Donethanks!

Copy link
Member Author

@gordlin gordlin left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 15 files reviewed, 3 unresolved discussions (waiting on @IshavSohal, @RyanCoulsonCA, and @szczz)


a discussion (no related file):
Latest push adds a bunch of minor fixes:

  • Changed the panel type name in the type change warning popup to uppercase
  • Implemented minor design changes in the map editor, slideshow editor, and video editor. They should now use the rounded UI elements, and layout better as the screen size changes.

As an aside, the CSS in the storylines editor is getting a bit fragmented. I've noticed a bunch of styles being re-implemented multiple times, and a couple hacky workarounds for styling problems that could be resolved by simplifying the stylesheets (so there aren't multiple conflicting styles for an element, for example). It might be a good future task to 'refactor and unify' the CSS in the app.

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.

Reviewed 5 of 6 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gordlin and @IshavSohal)


a discussion (no related file):

Previously, gordlin (Gordon Lin) wrote…

Latest push adds a bunch of minor fixes:

  • Changed the panel type name in the type change warning popup to uppercase
  • Implemented minor design changes in the map editor, slideshow editor, and video editor. They should now use the rounded UI elements, and layout better as the screen size changes.

As an aside, the CSS in the storylines editor is getting a bit fragmented. I've noticed a bunch of styles being re-implemented multiple times, and a couple hacky workarounds for styling problems that could be resolved by simplifying the stylesheets (so there aren't multiple conflicting styles for an element, for example). It might be a good future task to 'refactor and unify' the CSS in the app.

Just a couple more minor things:

  1. The caption input for charts is quite small, this can probably be made longer:
    image.png

  2. The spacing has been removed between the 'text' and 'panel' switch for dynamic panels which is causing the focus outline to overlap the other button.
    borderoverlap.gif

Copy link
Member Author

@gordlin gordlin left a comment

Choose a reason for hiding this comment

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

Reviewable status: 10 of 17 files reviewed, 2 unresolved discussions (waiting on @IshavSohal, @RyanCoulsonCA, and @szczz)


a discussion (no related file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

Just a couple more minor things:

  1. The caption input for charts is quite small, this can probably be made longer:
    image.png

  2. The spacing has been removed between the 'text' and 'panel' switch for dynamic panels which is causing the focus outline to overlap the other button.
    borderoverlap.gif

  1. Donethanks, I've modified the design of the chart item fields.

image.png

  1. Donethanks! I've changed the styling a bit so that the buttons look like a button group, which is what it was before the redesign. The outline still overlaps, but I think it makes sense now.

image copy 1.png

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.

Reviewed 7 of 7 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @IshavSohal)

Copy link
Member

@IshavSohal IshavSohal left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 14 files at r1, 2 of 6 files at r2, 3 of 4 files at r3, 7 of 7 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @gordlin)

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.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @gordlin)


a discussion (no related file):

Previously, gordlin (Gordon Lin) wrote…

Donethanks! Also moved the caption input to the top (above the image preview stuff) for easier access.

Good enhancements all-around. For the enhanced "full slide" functionality, I'm not able to get the "make current panel the full slide" popup to show up if the panel is empty. See below:

full-slide.gif

When the panel has some type of content, the behavior works as intended.

Copy link
Member Author

@gordlin gordlin left a comment

Choose a reason for hiding this comment

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

Reviewable status: 15 of 17 files reviewed, all discussions resolved (waiting on @IshavSohal and @RyanCoulsonCA)


a discussion (no related file):

Previously, yileifeng (Yi Lei Feng) wrote…

Good enhancements all-around. For the enhanced "full slide" functionality, I'm not able to get the "make current panel the full slide" popup to show up if the panel is empty. See below:

full-slide.gif

When the panel has some type of content, the behavior works as intended.

Donethanks!

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.

Reviewed 5 of 14 files at r1, 1 of 6 files at r2, 3 of 4 files at r3, 6 of 7 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @gordlin)

Copy link
Member

@spencerwahl spencerwahl left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 14 files at r1, 1 of 6 files at r2, 3 of 4 files at r3, 6 of 7 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @gordlin)

@yileifeng yileifeng merged commit 3ef2cd0 into ramp4-pcar4:main Jan 10, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Active PRs that require a fierce eyeballin. PR: Frontend PR that primarily involves frontend changes. UI experts and CSS Wizards are asked to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants