Size Change: +314 B (0%)

Total Size: 1.19 MB

_{compressed-size-action}

The behaviour here feels good to me, and the keyboard navigation is exactly as I would expect.

As mentioned elsewhere, I'd even go as far as removing the auto-focus on the first button inside the navigation (Back to Dashboard) on sidebar toggle, since now it's only as far as a simple tab press.
I'd defer this decision to a11y, though.

Most obvious change required: replace all the new z-index values with the z-index mixin, as mentioned in an inline comment.

About the InterfaceSkeleton change.

I don't see any obvious regressions, but I'm wondering if there's a way to tidy it up a little bit.
(Note that I haven't tested my propositions, so I'm not certain they would work 🙂)

• Is the interface-interface-skeleton__layout wrapper essential?
  Couldn't we just use the root interface-interface-skeleton as container?
  Off the top of my head, the reason for __layout is so that the interface-interface-skeleton__footer is not affected by the navigation sidebar sliding in.
  What if, instead, the sidebar "moved" the footer as well, exactly like it moves header and content?
  (Note that this is possibly kinda awkward, considering that status bars are usually full-width unaffected by any other UI element of the window.)

• How do you feel about interface-interface-skeleton__editor instead of interface-interface-skeleton__site?

• Is "navigation sidebar" a good and descriptive name for the new skeleton element?
  I wonder if "navigation" is too semantically specific, whereas I guess we should probably just convey an "interface" meaning (e.g. just "sidebar").
  For example, "Drawer" sounds like a good and descriptive name to me: it's established as a UI element (e.g. Google's Material), and it conveys a "sliding" movement, different from the other sidebars, who just simply appear.

Is the interface-interface-skeleton__layout wrapper essential?

Great catch! You were spot on about why I initially implemented skeleton-layout, but it turns out that we can remove the layout wrapper while still preserving current behavior. skeleton-footer has position fixed, which always attaches it to the bottom of the screen 🙂

How do you feel about interface-interface-skeleton__editor instead of interface-interface-skeleton__site?

editor is more semantically specific but it conveys a lot more meaning. I'm on board.

Is "navigation sidebar" a good and descriptive name for the new skeleton element?
I wonder if "navigation" is too semantically specific, whereas I guess we should probably just convey an "interface" meaning (e.g. just "sidebar"). For example, "Drawer" sounds like a good and descriptive name to me: it's established as a UI element (e.g. Google's Material), and it conveys a "sliding" movement, different from the other sidebars, who just simply appear.

Agreed. I'll give this more thought and circle back tomorrow. For now, I'll use drawer instead of navigation-sidebar.

Sorry! Those two commits are meant to be committed to a different branch 😬

No worries! Rebased the PR and dropped the rogue commits.

@Copons Thanks for merging this! 🙏

This PR appears to consider mobile (specificaly the iPhone based on the 335px variable introduced), but the mobile experience remains broken, so I'm curious how this was tested for mobile. For me, the site editor is just a white screen on mobile, something which #26021 fixes, but even after that fix, the 335px variable appears to not really address any mobile issues.

I really appreciate all the work you're doing on this, and I know it's super early to optimize for a breakpoint that is likely going to be rarely used for building templates. However this will nevertheless need to be considered at some point, and the sooner we start testing it, the less technical debt we will have to revisit and fix.

Most of the time it's it's enough to wrap desktop-specific affordances in a @include break-small() { ... } clause.

This PR appears to consider mobile (specificaly the iPhone based on the 335px variable introduced), but the mobile experience remains broken, so I'm curious how this was tested for mobile.

Thanks for your thoughts @jasmussen. They're much appreciated.

To clarify, although the PR appears to consider mobile (like the 335px variable), I didn't explicitly take mobile screens into account, which admittedly might be an oversight. I only ensured that the desktop behaved responsively up to the point where the tablet / mobile break-points take over. I've seen the mobile white screen for a while in site editor development, which is part of the reason why I didn't test behavior at smaller screen widths. I should've, however, been more proactive about calling out the unexpected behavior that you're addressing in #26021.

I really appreciate all the work you're doing on this, and I know it's super early to optimize for a breakpoint that is likely going to be rarely used for building templates. However this will nevertheless need to be considered at some point, and the sooner we start testing it, the less technical debt we will have to revisit and fix.

I think these are very valid points, especially if we plan on supporting mobile site editing in the future. It's tough developing for a mobile experience without an idea of what it will look like, but I agree that we can circumvent a lot of issues with desktop-specific affordances wrapped in breakpoint mixins.

It's tough developing for a mobile experience without an idea of what it will look like, but I agree that we can circumvent a lot of issues with desktop-specific affordances wrapped in breakpoint mixins.

Absolutely.

And in absence of full knowledge of that, I'd just test site editor PRs against a viewport that's <600px wide and see how broken it is. As we move forward, we can then raise the bar for what brokenness we're willing to accept.