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 the layout component to separate the UI from the content. #18044

Merged
merged 5 commits into from
Nov 20, 2019

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Oct 21, 2019

While working on #17838 I noticed that it's very hard to work on the Layout of the Editor.

The fact is there are so many things to think about:

  • mobile
  • fullscreen
  • with or without sidebar
  • WP admin header height changing
    ...

These create different constraints that are spread across different components in our code base and also mixed with a lot of unrelated items.

In this PR, I'm basically splitting the UI and the editor regions out of the Layout component. The idea is that the new EditorRegions takes care of how things are displayed on the screen (sidebar, main area, header, footer), gives a name and enable aria region navigation for these different areas and the Layout component fills the different regions for content.

A nice side effect here is that all these layouting constraints are now constraint in a single stylesheet making it very easy to maintain and reason about.

This change may have some impacts on classNames, zIndex, browsers so it needs to be tested properly but I'm hoping that it makes our code way more manageable.

The new EditorRegions component can also be easily made more flexible (allowing different names for the regions) and shared with other screens (Widgets...)

Thoughts?

@youknowriad youknowriad force-pushed the update/editor-ui branch 2 times, most recently from ab9eb62 to 80c0eac Compare October 23, 2019 09:11
@youknowriad youknowriad changed the base branch from master to add/block-breadcrumb October 23, 2019 09:11
@youknowriad youknowriad requested review from jasmussen and a team October 23, 2019 09:18
@youknowriad youknowriad self-assigned this Oct 23, 2019
@youknowriad youknowriad added [Type] Code Quality Issues or PRs that relate to code quality Needs Accessibility Feedback Need input from accessibility Needs Design Feedback Needs general design feedback. labels Oct 23, 2019
@youknowriad youknowriad marked this pull request as ready for review October 23, 2019 09:19
@jasmussen
Copy link
Contributor

This seems very solid. Works well for me in all horizontal breakpoints, with and without sidebar. However there are a couple of issues to solve, first one is easy:

Screenshot 2019-10-24 at 09 10 47

Fullscreen has extra topmargin.

Another small one is the sidebar doesn't extend to the bottom:

Screenshot 2019-10-24 at 09 17 24

The bigger issue is related to the navigation sidebar, and is visible in a number of situations:

  1. If the user has a lot of plugins installed that add top level menus
  2. If the user has a small screen (11 or 13 inch)
  3. If the user prefers a not-so-high viewport

Screenshot 2019-10-24 at 09 11 31

The left sidebar does not scroll.

This is a frustrating issue, because you'd think you could just add overflow: auto; to the navigation pane and let that scroll when necessary. Unfortunately this would prevent the flyout menus from working:

Screenshot 2019-10-24 at 09 15 08

Which means there are only really two ways of fixing allowing the left menu to have flyouts, and scroll. One solution is in master currently, and it works by allowing you to scroll the html element naturally, and then overlaying the entire Gutenberg window with position: fixed;. This is what's causing the double scrollbars — the right most is for the HTML content, the innermost is for the sidebar or the editing canvas if the sidebar is toggled off:

Screenshot 2019-10-24 at 09 19 14

This is not a very elegant solution, but the only other solution is to refactor the left navigation menu to not use flyouts, such as is proposed in https://core.trac.wordpress.org/ticket/47012.

@youknowriad youknowriad requested review from ellatrix and mkaz October 24, 2019 07:54
@jasmussen
Copy link
Contributor

The scrollbar issue seems to have been fixed!

Screenshot 2019-10-24 at 10 49 21

One more issue, I'm seeing a z-index issue on the mobile breakpoints:

content z index

The content is above the editor bar. But the good news is, the viewport scrolls correctly on mobile! Well done.

I think this is ready for wider review.

@mtias
Copy link
Member

mtias commented Nov 18, 2019

This seems to be working well and generally makes sense to me. The set of structural dependencies looks a bit weird, though — should the sidebar, header, footer components be moved within editor-regions? Do we expect these components to ever be used outside of an editor regions wrapper?

I could imagine possibly making our exports reflect the dependency: EditorRegion.Header.

@youknowriad
Copy link
Contributor Author

So yeah, right now the EditorRegions component is editor specific and contains the aria region names. Ideally, it becomes more flexible to allow it to be used outside the edit-post context (say edit-widgets...).

I think this PR is the first step in that direction, the API to make it more generic can be thought of separately.

@mtias
Copy link
Member

mtias commented Nov 20, 2019

Let's test this as soon as possible to detect any possible regressions.

@jasmussen
Copy link
Contributor

I think this may have caused a small regression with TwentyTwenty, and other themes that use a different background color than white:

Screenshot 2019-11-22 at 09 51 45

The div is no longer full-height:

Screenshot 2019-11-22 at 09 52 04

I'll leave a couple of code comments that I think may be the cause.

@@ -1,21 +1,6 @@
.edit-post-layout,
.edit-post-layout__content {
height: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

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

This one could be the cause of #18044 (comment).

Copy link
Contributor

Choose a reason for hiding this comment

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

This CSS appears to fix the issue:

.edit-post-visual-editor {
	height: 100%;
}

But this div also happens to be editor-styles-wrapper, and I'm not sure any heights should be applied there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems like a good fix to me, It was like that before so I don't see a reason for it to be a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Want me to PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

PR'ed: #18687

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you

@jasmussen
Copy link
Contributor

I think this PR might have regressed the z-index location. It's now in the top left corner:

Screenshot 2019-11-26 at 12 50 06

However #18732 might fix it.

@youknowriad
Copy link
Contributor Author

It's already fixed in master

</FocusReturnProvider>
<FocusReturnProvider className={ className }>
<EditorRegions
className={ className }
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary that className be applied on both the FocusReturnProvider and EditorRegions elements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's unintentional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Accessibility Feedback Need input from accessibility Needs Design Feedback Needs general design feedback. [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants