-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Try: Make site editor responsive. #26021
Conversation
This addresses feedback from #25134 (comment). |
Size Change: +65 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
@@ -86,11 +85,13 @@ html.interface-interface-skeleton__html-container { | |||
left: 0; | |||
background: $white; | |||
color: $gray-900; | |||
width: 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome catch, I'll need to address that! Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright this should be fixed.
e508a3d
to
96f531f
Compare
Alright this one should be ready to go. It would be really nice to get this one in soon, both so the experience isn't totally broken on mobile, but also so that actually optimizing for and considering mobile can be easier as development moves on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh how could I miss that !? Taking a look, thank you! |
I just pushed a commit to address this comment, which seemed to be causing layout issues for mobile. I think it might be able to hold us over until we rework the mobile experience altogether. The header at smaller viewport widths should now look like this: I pushed it here because I thought it would be easier to test, discuss, and potentially include with the fixes made in this pull request, but I'm more than happy to rebase and remove the commit and include it in separate PR. Would love to hear your thoughts @jasmussen |
Awesome. There are a lot of good CSS changes in this PR that I believe will hold us over for a while wrt the mobile site editor. But it's still blocked on the white screen, and I think I'll need some help getting that working. Behold: That white screen is actually the "left sidebar" being open, all the time. |
498b989
to
e95f5b4
Compare
7b78010
to
179d73f
Compare
I pushed changes and the WSOD should now be addressed. Would love to hear from @david-szabo97 @Copons @Addison-Stavlo @vindl if there are any concerns. I moved the logic to handle conditional rendering of the Left Sidebar up to its parent, |
Stellar work, @jeyip, I'm seeing this, and it's a substantial improvement: Thanks so much, I hope we can land this soon! |
Any reason to move it to the parent? I prefer it to be in a separate component so less clutter in the |
Good question! It's because the
In edit-site, however, we had the left sidebar set up in such a way that it was always truthy gutenberg/packages/edit-site/src/components/editor/index.js Lines 212 to 219 in 7778a36
This was leading to the unexpected behavior we were seeing with the mobile wsod. The
Hmmm...this is a good point. I thought about alternative approaches, but I don't see any immediate solutions. I think we'd have to remove interface skeleton entirely to address unnecessary re-render and the wsod? Would also love to hear if you have any ideas @david-szabo97 |
I wonder if such a skeleton refactor should block this PR from landing, given:
In that vein, we could consider this a regression fix with an opportunity to refactor the skeletons for the benefit of both editors? |
Ah I should have clarified @jasmussen. I completely agree with your sentiments. I think, in the worst case scenario, we should move forward with the changes in this PR for the reasons you listed above. I was interested in hearing from David to see if he had ideas about solutions that would meet both criteria of:
|
Since the implementation is identical to the Post Editor, I think we are good to go. I can't think of any easy way to fix the rerender for now. We will definitely need to rethink the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I noticed, we can open the Navigation sidebar by clicking on the name of the current template then clicking on "Browse templates". This opens the sidebar but there is no way to close it.
The name of the template in the header also gets pushed and it is no longer visible. (If you resize while the sidebar is open you will see that the name of the template is showing up once your browser is larger.)
After all, since this PR has been going on for a while, we would have a good foundation once this PR is merged, then we can address the issues coming up.
Thank you for the review David! I would agree there's very ripe ground for refactoring this, and it should be done.
This observation, I think, is one of the best reasons for moving ahead with this, even if it isn't optimal. This is the sort of bugfix that has to help inform the sidebar work, which we can only discover when it's in some state of functional. I'm going to give this a rebase, and see if that helps the checks pass. |
179d73f
to
c57e6f3
Compare
Flex bases of 335px were added to the left and right site editor header toolbars to prevent their children from collapsing when shrinking the viewport. This, however, did not account for tbehavior at smaller screen widths (tablet / mobile), and caused layout issues. To address this, we use breakpoints to define the 335px flex bases as desktop specific affordances.
The InterfaceSkeleton component coordinates the general layout and styling of the site editor and post editor pages. It expects specific behavior from its props. For example, if the LeftSidebar is open, InterfaceSkeleton expects to be passed JSX markup as the leftSidebar prop. Otherwise, if the LeftSidebar isn't open, InterfaceSkeleton expects a null or falsey leftSidebar prop. InterfaceSkeleton styling depends on this behavior. In edit site, the LeftSidebar prop passed into InterfaceSkeleton was always a truthy value. As a result, InterfaceSkeleton was improperly accounting for a LeftSidebar, even when it was unmounted from the screen. These changes move the logic to determine when the sidebar should "open" up from the LeftSidebar component itself to its parent. This allows us to pass JSX markup to the InterfaceSkeleton when the inserter is open, and a falsey value when the inserter is closed.
c57e6f3
to
af4c437
Compare
I rebased master again because there was still a failing (seemingly unrelated) e2e spec. |
All specs are now passing 👍 Going to go ahead and merge the PR.
Thanks for finding this @david-szabo97. I made an issue here to track the problem you pointed out.
I wonder if we want the interaction here to actually match the inserter, where, at smaller viewports, the sidebar actually occupies the entire screen. Either way, this isn't ideal either, and I made another issue to track this |
The site editor is currently not usable on mobile:
This PR does a number of things to improve that:
This improves things a fair bit:
I wouldn't call this the end of the road, there's a great deal of polish that reains possible:
But for now, this is a decent start.