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

Refactor editor layout to use grid #510

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gordlin
Copy link
Member

@gordlin gordlin commented Jan 15, 2025

Related Item(s)

Issue #503

Changes

  • [REFACTOR] Changes editor base layout to use grid instead of previous hardcoded solution. The editor layout should now be more robust, and should be able to 'flex' and adjust to element height changes without scrollbars appearing.

Notes

No scrollbar for the editor itself, despite the larger-than-usual header!
image

Testing

Steps:

  1. Open any product.
  2. Play around with the editor, especially with the slide list. No matter what you do (e.g. resizing the window, adding/copying slides, opening a specific panel's editor, etc.) the main editor should never get a scrollbar.
    a. (Note: There does seem to be a bug where scrolling through the slide ToC while a tooltip is showing can cause a scrollbar to appear for a short while before disappearing. If others get it too, it might be worth opening an issue for).

This change is Reviewable

@gordlin gordlin added the PR: Frontend PR that primarily involves frontend changes. UI experts and CSS Wizards are asked to review. label Jan 15, 2025
Copy link

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

@gordlin gordlin mentioned this pull request Jan 17, 2025
@gordlin gordlin force-pushed the editor-base-layout branch from d3d033d to 81acf99 Compare January 17, 2025 19:39
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.

Shrinking the browser size to a mobile(?) resolution does eliminate the editor scrollbar but also removes the page scrollbar. There is no way to access any of the slide content besides using keyboard navigation:

image.png

To reproduce, shrink the browser size until the mobile layout comes into view.

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


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

            // Commented out as it was causing issues with the "Edit Metadata" modal's config swap button.
            // Is it actually needed?

I'd assume this was code prior to the main editor redesign of having both lang slides side by side so would be safe to remove, but it'd be good for someone to confirm

@gordlin gordlin force-pushed the editor-base-layout branch from 81acf99 to a316bd4 Compare January 20, 2025 16:33
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.

This issue's been solved in the related PR #514, which is based on this branch - it was part of making the header sticky. I'd suggest doing both these PRs at once, I try to keep that one synced up with this one.

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


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

Previously, yileifeng (Yi Lei Feng) wrote…

I'd assume this was code prior to the main editor redesign of having both lang slides side by side so would be safe to remove, but it'd be good for someone to confirm

Sounds good. I'll wait a bit to remove the comment, just in case anyone else has something to chime in about this.

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 scrollbar issue does seem to be fixed in the latest PR. Noticed a new issue related to this PR with the Canada.ca templates on the metadata page, as the page footer is layered on top of the metadata content when a product is loaded:

image.png

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

@gordlin gordlin force-pushed the editor-base-layout branch from a316bd4 to 0dd33c9 Compare January 21, 2025 17:59
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.

Donethanks!

Reviewable status: 1 of 3 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.

Looks good now 👍

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


src/components/slide-toc.vue line 75 at r3 (raw file):

        <!-- Slide list -->
        <ul class="toc-slide-list" :class="[isMobileSidebar ? 'toc-list-mobile' : 'toc-list']">

'toc-list-mobile' and 'toc-list' can be removed with these changes?


src/components/slide-toc.vue line 677 at r3 (raw file):

}

.line-clamp-2 {

Is this used anywhere?

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 1 of 3 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gordlin and @yileifeng)


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

Previously, gordlin (Gordon Lin) wrote…

Sounds good. I'll wait a bit to remove the comment, just in case anyone else has something to chime in about this.

Can't remember what exactly this bit of code was for, but I'd say give removing it a shot and see if anything breaks.

@gordlin gordlin force-pushed the editor-base-layout branch from 0dd33c9 to 9ab2447 Compare January 22, 2025 17:28
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: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @RyanCoulsonCA and @yileifeng)


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

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

Can't remember what exactly this bit of code was for, but I'd say give removing it a shot and see if anything breaks.

Donethanks!


src/components/slide-toc.vue line 75 at r3 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

'toc-list-mobile' and 'toc-list' can be removed with these changes?

Donethanks!


src/components/slide-toc.vue line 677 at r3 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

Is this used anywhere?

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.

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @yileifeng)


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

Previously, gordlin (Gordon Lin) wrote…

Donethanks!

Things seem to be working for me still. I actually think you might be able to remove this entire if block, since loadConfig also calls updateSlides (via useConfig function).

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 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gordlin)

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 1 of 2 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gordlin)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants