Size Change: +1.63 kB (0%)

Total Size: 1.11 MB

_{compressed-size-action}

Thanks for exploring this @kevin940726. I've not yet reviewed the code closely, but it looks simpler than I thought. I like that we're using history instead of a custom approach. I like that history adds +923 B which isn't so bad. I like that everything is compartmentalised to edit-site so that we can iterate on the APIs here without impacting too many bits of Gutenberg.

I gave it for a spin and it works very well. Here's a video. I throttled network requests down to WiFi speeds for a more realistic example.

My only suggestion is that I think we should automatically collapse the black navigation menu on the left when you open a template or template part.

My only suggestion is that I think we should automatically collapse the black navigation menu on the left when you open a template or template part.

Yeah, I think this is a bug. Everything becomes more complicated in SPA 😅 .

We obviously still need to pay extra attention to accessibility to ensure that it's still accessible.

I did a little bit of testing in Voiceover, and when selecting a link nothing is announced and that might lead a screenreader user to believe that nothing happened.

It looks like the page title isn't being updated when navigating, and that might be one of the causes, though I'm not sure if it would fix it.

I think we also need to handle the 'You have unsaved changes' warnings in the editor in a client side routing model. Usually that's tied into the page unload, but that doesn't happen now.

Steps to repro:

1. Make some changes to a template using the editor
2. Switch to the 'Templates' page in the sidebar
3. Go to the dashboard

Expected: At some point (probably during step 2), a warning should show about unsaved changes, asking the user to confirm or cancel before navigating.
Actual: No warning is shown.

Just leaving a note to say: 1) thank you! This will be a much better UX than full page refreshes when navigating between the Editor and template list views

And 2) this fixes the issues we've found on WP.com around iframing the Site Editor.

I did face one JS error that broke the whole editor when switching to the 404 template but I wasn't able to reproduce consistently, maybe that screenshot can help pin point the issue somehow.

That seems unrelated. There are some code smell regarding not properly cancelling requests when a component unmounts. These could be more visible when the user can now navigate to another page and unmount everything on previous page, while in the past they can only reload the page and nuke everything (including the warnings and errors). Fixing them could be a larger issue though.

Potentially we could just use the navigator components and if we really need to keep the "URL" routing, we add a layer (component or hook) of synchronization between the history and the navigator.

I haven't really thought through this, but I have a hunch that the source of truth should always be the url in CSR. Synchronizing could be really complicated, and history just handles it so well for us and battle-tested in popular routing libraries. I'm not against of this idea, but perhaps we might have to re-think the best practices in navigator components if we ever want to migrate to using it.

If we do this for template parts, shouldn't we also do it for the templates themselves? 🤔

Yep, that's the plan :).

Another small issue I noticed is that the "Site" link in the W menu seems to be active regardless of what I'm editing...

Yep, this is a known bug tracked in #36821. I don't think this PR changes any of that?

It would be great if the switch announcement could be something like "Now displaying: template/template_part."

@alexstine Interesting! I'm not seeing popular frameworks doing this though. Is there some references or articles about this? I want to learn more about the best practices :).

For now, how easy is it to simply put focus on the navigation button? It's the first element on the page, so should be a compromise without needing Skip to Content.

Probably not too difficult, probably just some edge cases needed to be handled. I'll try!

We're not really discussing whether the old or new behavior is better. The issue is that there's currently no warning when exiting via the template list page. So a user would lose unsaved changes.

Oh got it, isn't this just a matter of adding the unsaved changes component to the list view (or moving it un in the react tree)?

I haven't really thought through this, but I have a hunch that the source of truth should always be the url in CSR.

In my opinion, the applications that rely on the router and URL for the source of truth leak too much information about routing to the UI components. I personally see routing as a side effect and I don't like splitting the source of truth between different places.

Oh got it, isn't this just a matter of adding the unsaved changes component to the list view (or moving it un in the react tree)?

Yeah, I think that would be the simplest fix and easiest thing to do right now. It seems like the alternatives require a lot of changes.

@kevin940726

@alexstine Interesting! I'm not seeing popular frameworks doing this though. Is there some references or articles about this? I want to learn more about the best practices :).

I am sure you can find plenty of examples, but this really just comes down to common sense. If you were a blind user that selected the Header template and you heard "Editor", what would that mean to you? Wouldn't it make more sense to hear "Now editing Header template."? I already know I'm in the Site Editor, I don't need to hear that every time a template/template part is selected.

Thanks.

@alexstine

I am sure you can find plenty of examples, but this really just comes down to common sense. If you were a blind user that selected the Header template and you heard "Editor", what would that mean to you? Wouldn't it make more sense to hear "Now editing Header template."? I already know I'm in the Site Editor, I don't need to hear that every time a template/template part is selected.

I understand and I agree! I'm just wondering when will it differ from the traditional multi-page navigation? As far as I understand, it won't announce "Now displaying" when navigating to a different page by default either. I understand the experience in the Site Editor is a bit different though (and probably that's the reason), I'm just curious and want to learn more about this topic 😅 .

@talldan I just added <UnsavedChangesWarning> to the list page! It now warns if we have unsaved changes when trying to leave the list page, but doesn't prevent us from leaving the editor.

For now, how easy is it to simply put focus on the navigation button? It's the first element on the page, so should be a compromise without needing Skip to Content.

Turns out this is not that easy as I thought, might requires some refactoring. I'll probably do this as a follow-up PR, and add this to #36597 for tracking and visibility.

On this issue with the Dashboard button, it does seem to happen only in this PR from what I can tell:

It's related to these styles:

Here, .components-button.is-tertiary overrides the MenuBackButtonUI.is-tertiary rule. In trunk it's the other way around.

@kevin940726 The biggest reason this needs to happen is because people with sight can simply check the page and see that you changed to new content. Without specifically providing this update to screen reader users, they may wonder why nothing happened. Take Google Calendar for example. If I want to view my events for January, 2022, I simply click on the Next Month button. Then it tells me that it is displaying events for January, 2022. Why? Because of lack of page reload. Screen reader users would have no idea the content has changed unless they went out of their way to check the content of the page.

This doesn't really apply to our situation fully, but it's better than nothing.

https://www.w3.org/TR/UNDERSTANDING-WCAG20/consistent-behavior-no-extreme-changes-context.html

I am thinking WCAG 3.0 will come up with better rules for dynamic content. Remember, accessibility is generally always dreadfully behind the times, screen readers and dynamic content are no exception to the rule.

Here is another great resource for the why factor.

https://accessibility.huit.harvard.edu/provide-notification-dynamic-changes-content

Also, the note about focus. Usually, changing focus isn't a great idea but dynamic web apps are changing this. In my opinion, it is much better to always plop users in a central location after an action such as a template/template part selection. In this case, the main navigation trigger would be a great global point.

I understand these should be addressed in another PR as this one is getting large, but I also want to make very clear that these should be considered blockers for the 5.9 inclusion. I really like the simplicity of not having the page reload, but we've got to make sure screen reader users don't get disoriented when the page dynamically updates from their selection.

PS: apologies for a gruff sounding reply earlier. I should not be replying on these before I've had coffee. 👎 for me.

Thanks.

@alexstine Thank you! These are really helpful! 💯

I just pushed a commit to add "Now displaying" to the page navigation announcement in 6b38d67. Hope that I'm not misunderstanding it.

I understand these should be addressed in another PR as this one is getting large, but I also want to make very clear that these should be considered blockers for the 5.9 inclusion. I really like the simplicity of not having the page reload, but we've got to make sure screen reader users don't get disoriented when the page dynamically updates from their selection.

100% agree! I added the focus management task to the tracking issue as well (under "Accessibility").

Let's move further discussions there to better track them and create new issues if needed.

PS: apologies for a gruff sounding reply earlier. I should not be replying on these before I've had coffee. 👎 for me.

No worries! I'm always grateful to have your inputs on accessibility feedbacks. I'll try to learn more to keep our collaboration smoother ♥️ !

@kevin940726 Looks good. Thanks! 👍 💯

Thanks @kevin940726! Really nice collaborative effort here everyone 👏