Site Editor: Make sidebar back button go *back* instead of *up* if possible #52456
Conversation
noisysocks
added
[Type] Enhancement
|
Size Change: +27 B (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
packages/components/src/navigator/navigator-provider/component.tsx
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/sync-state-with-url/use-sync-path-with-url.js
Outdated
Show resolved
Hide resolved
noisysocks
added
[Type] Bug
There was a problem hiding this comment.
Very cool! In general this was testing pretty well for me, but I ran into an issue where sometimes the navigation didn't appear to correctly work when switching between pages. I couldn't reproduce the issue on trunk. I'll try to describe what I was seeing in case it's just in my environment:
2023-07-10.16.12.33.mp4
- Sometimes when switching between pages, the navigation doesn't appear to take effect
- Sometimes instead of sliding in, the page fades in (in the left sidebar)
I only noticed these issues while navigating between pages in the site editor. In the above example, the page has a featured image, in case that affects things at all.
Other than that, the new back behaviour was otherwise testing really well!
Good spotting. I can reproduce now and then, but only when I click on a page, go back, then click on the same page. 
|
|
Personally, I still think this is the wrong behavior as I explained on the issue. Here's a flow that proves this is wrong:
That's too confusing. |
|
@youknowriad that isn't the expected behavior and should be addressed as a part of this work. The idea isn't to "go back rather than up", it is to go up but contextually based on the user flow. It's more like an internal breadcrumb that gets built as the user traverses downwards through the navigator. The < button should take you to the direct ancestor in that breadcrumb. 99% of the time the user flow will match the built-in logic, but when routes cross over (e.g. Pages → 404 template) we need a way for users to return to the original panel. As seen in the feedback, the expectation is that the < button would do exactly this. |
|
Thanks for testing everyone. I attempted to simplify |
There was a problem hiding this comment.
Set GitHub to "hide whitespace" when reviewing this file.
There was a problem hiding this comment.
The link if anyone is interested: https://github.com/WordPress/gutenberg/pull/52456/files?w=1
|
Flaky tests detected in e83e1af. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5538182134
|
| @@ -36,6 +36,12 @@ export function getPathFromURL( urlParams ) { | |||
| return path; | |||
| } | |||
|
|
|||
| function isSubset( subset, superset ) { | |||
There was a problem hiding this comment.
Just curious: this function, on the surface, appears to check equality. Is it possible to add a note to explain the relationship between subset and superset?
Or just in this comment box for me 🤣
There was a problem hiding this comment.
If all of the key/value pairs of A are also present in B then A is a subset of B and B is a superset of A.
https://en.wikipedia.org/wiki/Subset
Maybe I should call it includes which is what lodash used? Would that be more familiar?
There was a problem hiding this comment.
Maybe I should call it includes which is what lodash used? Would that be more familiar?
I was only thinking of the casual observer, especially since this file's code is, or at least has been, at best, difficult to grok at first glance.
I'm fine with the function description personally. Thanks for the explainer! 🍺
There was a problem hiding this comment.
Checked behaviour with "W" icon (#52456 (comment))
Checked clicking on same page item (#52456 (review))
Works on template redirects, 404, blog home from the pages list
Navigating up and down the navigation trees works from all items.
2023-07-11.13.46.04.mp4
I love it! Thank you @noisysocks
|
This is definitely testing better than it was for me yesterday as it appears to resolve the issue of clicking in and out of the same page 👍 One issue I ran into is that if I hit the browser's back button at any point, it seems to flip the navigation's back button over to going through history state instead of up the breadcrumb hierarchy? This means that after hitting the browser's back button once, it's possible to get stuck in a bit of a loop going through pages you've already viewed, instead of going back to the root. Here's a screengrab: 2023-07-11.13.56.12.mp4 |
|
@andrewserong: Hmm, you're right that it's confusing. To fix it we'd need to make the back button literally call Fixing it would also result in a confusing disparity where if you e.g. navigate to Pages → Contact, press back, and then press the browser's back button you'll exit the site editor. But if you navigate directly to Contact, press back, and then press the browser's back button you'll end up at Contact. To be honest I think the clearest thing we can do is show breadcrumbs as in this mockup and then either a) have a back button that exactly duplicates the browser's back button; or b) rely entirely on the browser's back button. |
|
Thanks for working through the options @noisysocks, this is a tough one!
I really like the look of that mockup, I think breadcrumbs could work really nicely. |
|
I kept hacking on this, and have a few ideas, but it's late in the day. I'll open some PRs that explore alternatives tomorrow. |
| // Trigger only when URL changes to prevent infinite loops. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [ urlParams ] | ||
| ); |
There was a problem hiding this comment.
Can you clarify the changes in this hook. The hook is supposed to be independent of the navigation, just synchronizes changes, so I'm not sure how it's related to this PR
There was a problem hiding this comment.
Yep so the key change, and all that this PR needs at minimum, is changing this:
To this:
The former is buggy. When something calls navigator.goTo, e.g. when you click Pages, the result is:
- The navigator path changes to
/page - The first
useEffectfires (because the navigator path changed) and callshistory.pushto change the URL to?path=%2Fpage - The second
useEffectfires (because the URL params changed) and callsnavigator.goToto navigate to/page
(3) is extraneous. We don't need to call navigator.goTo( '/page' ) as the navigator location is already /page. Doing this results in two history items within the navigator and means that the user has to click back twice to return to the root page.
The simplest way to fix this would be to update currentPath.current in the first useEffect, but I thought I'd go further and attempt to simplify useSyncPathWithURL to not require the currentPath and currentUrlParams refs at all. They duplicate state that is already in the useHistory and useNavigator hooks. Having the refs makes the hook more difficult to understand and makes it easy for bugs of this nature to slip by.
noisysocks
force-pushed
the
update/make-sidebar-back-button-go-back
branch
from
b02c0e5
to
e83e1af
Compare
|
I explored a few different techniques that incorporate the browser's back button but didn't really find anything I was very happy with. Speaking with @SaxonF, we think it's okay that pressing the back button in the sidebar creates new browser history entries. That is to say, what @andrewserong noticed above is expected. I've pushed a few changes:
|
|
This is working great for me. The only browser back button curiosity I could find was when clicking the browser's back button on a third-level item, e.g., "Page" or Template. The new browser entries gives the impression of a loop. It is possible, however, to "back" out of it. Trunk2023-07-13.13.00.12.mp4This PR2023-07-13.13.00.39.mp4All other interactions with the back button, as far as I could discern behave as my brain would expect, which is to say I wasn't scratching my chin when randomly clicking the arrow and the browser's back button. On the other hand, this PR improves greatly the logical consistency of navigating between Pages, whose child items are Templates. This is, in my opinion, more glaring and more likely to surface. Trunk2023-07-13.13.02.25.mp4This PR2023-07-13.13.17.39.mp4Good to go? |
|
Yeah let's expose this to more people and see what comes of it. |
|
Here's another use-case where for me this doesn't make sense.
Instead of going to the "pages" sidebar, you go to the previous template that was opened. That feels wrong to me, If I wanted that I would have clicked the "previous" button of the browser why duplicate this one in the UI. |
…dd/defer-script-loading-strategy * 'trunk' of https://github.com/WordPress/gutenberg: (24 commits) Add filter to turn off Interactivity API for a block (#52579) Search: Remove unnecessary useEffect (#52604) Navigation: Simplify the useSelect for useNavigationMenus (#51977) Item: Unify focus style and add default font styles (#52495) Update Changelog for 16.2.1 Bump plugin version to 16.2.1 Avoid passing undefined `selectedBlockClientId` in `BlockActionsMenu` (#52595) Cover Block: Fix block deprecation when fixed background is enabled (#51612) Nav block: link text color inheritance fixes and tests (#51710) Stabilize defaultBlock, directInsert API's and getDirectInsertBlock selector (#52083) Fix console warning by improving error handling in Nav block classic menu conversion (#52591) Fix: Remove link action of Link UI for draft pages created from Nav block does not correctly remove link. (#52415) Lodash: Remove remaining `_.get()` from block editor and deprecate (#52561) Fix importing classic menus (#52573) ResizableFrame: Make keyboard accessible (#52443) Site Editor: Fix navigation menu sidebar actions order and label (#52592) correct a typo: sapce -> space (#52578) Avoid errors in Dimension visualizers when switching between iframed and non-iframed editors (#52588) Patterns: Add client side pagination to patterns list (#52538) Site Editor: Make sidebar back button go *back* instead of *up* if possible (#52456) ...
…ssible (#52456) * Navigator: Add replace option to goTo() and goToParent() * Site Editor: Make sidebar back button go back instead of up if possible
tellthemachines
removed
the
Backport to WP 6.7 Beta/RC
|
Cherry-picked for 6.3. |
* Site Editor: Restore quick inserter 'Browse all' button (#52529) * Site Editor: Restore quick inserter 'Browse all' button * Remove leftover comment * Patterns: update the title of Pattern block in the block inspector card (#52010) * Site Editor Pages: load the appropriate template if posts page set (#52266) * This commit: - links the posts page to the homepage template when a post page is set - abstracts logic to get page item props * The Posts Page resolves to display the Home or Index template only. Adding a check to skip the Front Page * Showing homepage settings for posts pages that are set as the post page in reading settings * Post pages that have been set to display posts will redirect to first the home template, then the index template. The fallback is the post id of the page. * Reverted refactor of packages/edit-site/src/components/sidebar-navigation-screen-page/index.js Will do it in a follow up * Allow editing existing footnote from formats toolbar (#52506) * Block Editor: Display variation icon in the 'BlockDraggable' component (#52502) * Patterns: add option to set sync status when adding from wp-admin patterns list (#52352) * Show a modal to set sync status if adding pattern from pattern list page * Make sure the modal loads if post settings panel not open * don't load modal component at all if not new post * Simplify the sync status so undefined always = synced * Update packages/editor/src/components/post-sync-status/index.js --------- Co-authored-by: Ramon <ramonjd@users.noreply.github.com> * Revise LinkControl suggestions UI to use MenuItem (#50978) * Use "link" instead of "URL" for URL_TYPE * Use MenuItem for search create button * Use sentence case for "Create page" * Use a MenuGroup for search results * Use MenuItem for search item * Refactoring styles (WIP) * Preserve whitespace in results text * Reinstate result item information including permalink * Remove debugging CSS code * Reinstate CSS to control size of rich previews favicon * Remove other commented out CSS code * Reinstate selected styles * Remove more redundant CSS * Add some basic results hover/focus styling. Needs improving * Improve icon alignment * Update tests to handle wording changes * Remove inconsistent hover/focus style MenuItem already has hover/focus styles * Reinstate is-selected visual state * Update test to make sense in context of #51011 See #51011 * Fix locator for result text --------- Co-authored-by: Dave Smith <getdavemail@gmail.com> * Fix LinkControl mark highlight to bold (#52517) * Add 'reusable' keyword to Pattern blocks (#52543) * Fix missing Add Template Part button in Template Parts page (#52542) * Tweak copy for the reusable block rename hint (#52581) * Trim footnote anchors from excerpts (#52518) * Trim footnote anchors from excerpts * Add comments, fix spacing, appease linter * Site Editor: Reset device preview type when exiting the editing mode (#52566) * Make "My patterns" permanently visible (#52531) * Hide site hub when resizing frame upwards to avoid overlap (#52180) * Fix "Manage all patterns" link appearance (#52532) * Fix "Manage all patterns" link * Update focus style * Update navigation menu title size & weight in detail panels (#52477) * Update menu title size * Adjust font weight * Site Editor Patterns: Ensure sidebar does not shrink when long pattern titles are used (#52547) * Edit Site: Select templateType from the store inside the useSiteEditorSettings hook (#52503) * Add width to ensure ellipsis is applied (#52575) * Cover Block: Fix block deprecation when fixed background is enabled (#51612) * ResizableFrame: Make keyboard accessible (#52443) * ResizableFrame: Make keyboard accessible * Fix outline in Safari * Use proper CSS modifier * Add aria-label to button * Keep handle enlarged when resizing (Safari) * Add back visually hidden help text * Don't switch to edit mode * Make the handle a role="separator" * Revert to `button` * Switch description text to `div hidden` * Prevent keydown event default when right/left arrow * Change minimum frame width to 320px * Mention shift key in description text * Only render resize handle when in View mode * Fix importing classic menus (#52573) * use the same create hook for classic import * Remove redundant arg to hook --------- Co-authored-by: Dave Smith <getdavemail@gmail.com> * Site Editor: Fix navigation menu sidebar actions order and label (#52592) * Avoid errors in Dimension visualizers when switching between iframed and non-iframed editors (#52588) * Patterns: Add client side pagination to patterns list (#52538) Co-authored-by: Saxon Fletcher <saxonafletcher@gmail.com> * Site Editor: Make sidebar back button go *back* instead of *up* if possible (#52456) * Navigator: Add replace option to goTo() and goToParent() * Site Editor: Make sidebar back button go back instead of up if possible * Adapt template part hint copy (#52527) * Try "panel" instead of "page" * Update template-part-hint.js --------- Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com> Co-authored-by: Glen Davies <glendaviesnz@users.noreply.github.com> Co-authored-by: Ramon <ramonjd@users.noreply.github.com> Co-authored-by: Miguel Fonseca <150562+mcsf@users.noreply.github.com> Co-authored-by: Rich Tabor <hi@richtabor.com> Co-authored-by: Dave Smith <getdavemail@gmail.com> Co-authored-by: Robert Anderson <robert@noisysocks.com> Co-authored-by: James Koster <james@jameskoster.co.uk> Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Co-authored-by: arthur791004 <arthur.chu@automattic.com> Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> Co-authored-by: Lena Morita <lena@jaguchi.com> Co-authored-by: Andrei Draganescu <me@andreidraganescu.info> Co-authored-by: Saxon Fletcher <saxonafletcher@gmail.com>
What?
Fixes #50676.
Adjusts the back button in the site editor (left) sidebar to go back to the previously seen screen if possible instead of up to the parent page in the hierarchy.
Why?
Going up results in confusing flows for users. For example, if you select Pages → 404 and then press the back button you'll be taken to Templates instead of Pages. See #50676 (comment) for a complete list of the currently confusing flows.
I strongly recommend reading the discussion in #50676.
How?
goBack()if there is an item to go back to instead ofgoToParent()goTo()is called twice: once when user clicks button, and then again when browser URL changesNavigatorchanges:AddhasBackproperty which is true whengoBack()will do somethingReset navigation history whengoToParent()is called or whenisBackis passed togoTo(){ replace: true }to be passed togoTo()orgoToParent(). This results in a new history item not being createdTesting Instructions