Add client side routing for Site Editor #36488
Conversation
|
Size Change: +1.63 kB (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
kevin940726
force-pushed
the
add/edit-site-list
branch
from
1d746ab
to
28e7eb3
Compare
kevin940726
force-pushed
the
add/edit-site-routes
branch
3 times, most recently
from
f2e99a5
to
8944e9d
Compare
|
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 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. Kapture.2021-12-03.at.15.51.06.mp4My 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 😅 . |
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:
Expected: At some point (probably during step 2), a warning should show about unsaved changes, asking the user to confirm or cancel before navigating. |
| */ | ||
| import { addQueryArgs } from '@wordpress/url'; | ||
|
|
||
| const history = createBrowserHistory(); |
There was a problem hiding this comment.
I believe, in theory, this PR should work even inside iframes. If for some reasons url transitions are completely off the table, we can still switch this to createMemoryHistory and get the similar effect.
kevin940726
force-pushed
the
add/edit-site-routes
branch
from
bed34dc
to
5a14c53
Compare
|
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. |
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. |
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. |
There was a problem hiding this comment.
It looks good so far. It'd be good to get a summary of what's remaining.
Shall we try the simplest possible fix for the unsaved changes issue like Riad suggested (using the UnsavedChangesWarning in the list screen)? All the other options seem to be really complicated!
| showHomepage().then( () => { | ||
| if ( ! isMounted ) { | ||
| return; | ||
| } | ||
|
|
||
| window.history.replaceState( {}, '', newUrl ); | ||
| }, [ pageContext ] ); | ||
| const page = getPage(); | ||
| const editedPostId = getEditedPostId(); | ||
| const editedPostType = getEditedPostType(); | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| function useCurrentPageContext() { | ||
| return useSelect( ( select ) => { | ||
| const { getEditedPostType, getEditedPostId, getPage } = select( | ||
| editSiteStore | ||
| ); | ||
|
|
||
| const page = getPage(); | ||
| let _postId = getEditedPostId(), | ||
| _postType = getEditedPostType(); | ||
| // This doesn't seem right to me, | ||
| // we shouldn't be using the "page" and the "template" in the same way. | ||
| // This need to be investigated. | ||
| if ( page?.context?.postId && page?.context?.postType ) { | ||
| _postId = page.context.postId; | ||
| _postType = page.context.postType; | ||
| if ( page?.context?.postId && page?.context?.postType ) { | ||
| history.replace( { | ||
| postId: page.context.postId, | ||
| postType: page.context.postType, | ||
| } ); | ||
| } else if ( editedPostId && editedPostType ) { | ||
| history.replace( { | ||
| postId: editedPostId, | ||
| postType: editedPostType, | ||
| } ); | ||
| } | ||
| } ); |
There was a problem hiding this comment.
So this is where there's a client side redirection? (#36873). I understand this is due to be replaced, is that going to be in 5.9?
I think this is a bit of tech debt because of how in all other cases the store state is reactive, but here it gets set first and the call to history.replace happens after. As long as it works right now, it's ok to leave it as is in this PR.
If we can't solve #36873, there might be a few things that can be done to tidy it up. I was wondering if it could be a selector like getHomepage, and then we handle routing based on what that returns. The state could then update as a result of that routing. Also feels like it could be done before the React App mounts to avoid any other effects triggering.
As a side note, the request to /wp/v2/settings that happens from showHomepage is possibly not using the preloaded data. I'm still seeing a network request in my dev tools. It probably wasn't an issue introduced here though, so let's follow up on that.
There was a problem hiding this comment.
I understand this is due to be replaced, is that going to be in 5.9?
I don't think so, unless someone is willing to work on that.
I was wondering if it could be a selector like
getHomepage, and then we handle routing based on what that returns. The state could then update as a result of that routing. Also feels like it could be done before the React App mounts to avoid any other effects triggering.
Sounds interesting! Would you mind try implementing this? Either push directly to this PR or to a different PR.
There was a problem hiding this comment.
I'm not sure of all the permutations, but I'll give it a go 😄
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 |
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, |
|
@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.
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.
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 👏 |
noisysocks
removed
the
Backport to WP Beta/RC
* Add client side router * Use saveEntityRecord to update the store * Fix updating state in unmounted components * Update titles when page navigates * Use slot/fill for NavigationSidebar * Announce title change * Inline all api requests * Fix php lint errors * Make title announcement assertive * Fix infinite loop in page navigation * Fix duplicate main roles * Fix not announcing same titles * Try to fix url query controller * Add UnsavedWarning to list page too * Add now displaying to use-title's speak
|
|
||
| // We dispatch actions and update the store synchronously before rendering | ||
| // so that we won't trigger unnecessary re-renders with useEffect. | ||
| { |
There was a problem hiding this comment.
I know this has been merged already, but I'm quite curious - why is the code below wrapped in this block?
There was a problem hiding this comment.
It's just for readability sake, trying to wrap similar things into a block for clarity.
| return state.editedPost[ state.editedPost.length - 1 ] || {}; | ||
| } | ||
|
|
||
| function getPreviousEditedPost( state ) { | ||
| return state.editedPost[ state.editedPost.length - 2 ] || {}; | ||
| return state.editedPost; |
There was a problem hiding this comment.
@kevin940726: in the interest of YAGNI, can we remove getCurrentEditedPost and let the selectors access state directly? As per before https://github.com/WordPress/gutenberg/pull/34732/files#diff-94ccd649d2e3a0efc85ceb76fc0ebf13e6b721cedec21edc917064fe4cb2c79bL143.
There was a problem hiding this comment.
Yeah sure. We needed it because we had getPreviousEditedPost which made it confusing. Now that we don't have that we can remove this intermediate function.
Description
Close #37075. Based on #36379. Part of #36597.
Add client side routing for the site editor to improve navigation performance.
This is not based on a traditional routing rule though, the path is determined by query params, which is how WP works currently.
This PR also changes how #34732 works by leveraging the router we have now to control the back button.
We obviously still need to pay extra attention to accessibility to ensure that it's still accessible.
How has this been tested?
tt1-blocksScreenshots
Kapture.2021-12-03.at.11.36.36.mp4
Types of changes
New feature
Checklist:
*.native.jsfiles for terms that need renaming or removal).