Add "Show block breadcrumbs" preference #28133
Conversation
|
Size Change: +1.75 kB (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
david-szabo97
requested review from
ajitbohra,
nerrad,
ntwb and
talldan
as code owners
david-szabo97
requested review from
mattwiebe,
vindl,
jeyip,
noahtallen,
Addison-Stavlo,
Copons and
shaunandrews
There was a problem hiding this comment.
This seems to be working well.
My main critique is having these disabled by default. Some users do definitely rely on this out of habit, and loading the editor after an update to fumble around to figure out how to turn it back on may not be the best experience. Beyond that, there has been suggestions for new user tours to point out the block breadcrumbs. Considering all of that I personally think it makes the most sense to have these breadcrumbs enabled by default.
| hasReducedUI: select( editPostStore ).isFeatureActive( | ||
| 'reducedUI' |
There was a problem hiding this comment.
I am wondering why we are removing this selector, and if we should still hide the footer if this reducedUI setting is true ? Regardless of whether or not the breadcrumbs preference is toggled, reducedUI should still probably hide the footer. Perhaps we should also only allow/show toggling breadcrumbs preference if we are not in the state of hasReducdedUI. 🤔
All that said, I don't have much knowledge about how/when hasReducedUI is used.
There was a problem hiding this comment.
All that said, I don't have much knowledge about how/when hasReducedUI is used.
Me neither. I prefer "block breadcrumbs" to be toggled separately. It's more flexible. If enabling "reduced UI" would disable the breadcrumbs as well, then that would lead to confusion IMO.
There was a problem hiding this comment.
More to the point - it looks like reducedUI used to hide the footer. After these changes it no longer will. Im assuming we still want reducedUI to have the same behavior regardless of this change?
I guess the possible states could be:
- before this change -
reducedUIhides footer regardless of breadcrumb - combination of
reducedUIand! blockBreadcrumbhides footer. This would require settingblockBreadcrumbfalse when loading the editor with thereducedUIsetting to preserve the same experience for people using that setting (i.e. otherwise the footer would randomly start showing up for people runningreducedUIsince it sounds like we are enabling the breadcrumb setting by default? ). - current state of PR -
blockBreadcrumbsetting itself hides/shows the footer regardless ofreducedUI
The first seems the most safe since that follows the previous behavior. The third (current state) seems risky to mess up the intentions of reducedUI. The middle option is more of a compromise between the two.
I prefer "block breadcrumbs" to be toggled separately. It's more flexible. If enabling "reduced UI" would disable the breadcrumbs as well, then that would lead to confusion IMO.
True. Im getting at IF we are going with the "before this change" behavior noted above, then going to toggle the block breadcrumb with the footer still hidden wouldn't do anything since its in the reduced UI state and the footer is hidden. So my suggestion then would be to hide the breadcrumb toggle while under the state of reducedUI, not to necessarily change its setting/preference.
There was a problem hiding this comment.
More to the point - it looks like reducedUI used to hide the footer. After these changes it no longer will. Im assuming we still want reducedUI to have the same behavior regardless of this change?
To add to this, given that we also need to show breadcrumbs by default for the time being, this will cause them to appear after the plugin update for users that were not seeing them before (if they had reduced UI enabled)?
There was a problem hiding this comment.
To add to this, given that we also need to show breadcrumbs by default for the time being, this will cause them to appear after the plugin update for users that were not seeing them before (if they had reduced UI enabled)?
Correct. Breadcrumbs would be visible for people who had reduced UI enabled.
The first seems the most safe since that follows the previous behavior. The third (current state) seems risky to mess up the intentions of reducedUI. The middle option is more of a compromise between the two.
That's a good point about making sure reducedUI has the same behavior. I'll push a commit to re-add reducedUI check.
|
The setting does what is should. Nice work. While I think the breadcrumbs are mostly ignored (they're hard to notice), they are currently the best way to navigate update the document. Until we have a better way (like #27395), its probably best to keep the default to showing the breadcrumbs. We can always change the default in another PR. |
| hasReducedUI: select( editPostStore ).isFeatureActive( | ||
| 'reducedUI' |
There was a problem hiding this comment.
More to the point - it looks like reducedUI used to hide the footer. After these changes it no longer will. Im assuming we still want reducedUI to have the same behavior regardless of this change?
To add to this, given that we also need to show breadcrumbs by default for the time being, this will cause them to appear after the plugin update for users that were not seeing them before (if they had reduced UI enabled)?
packages/interface/src/components/interface-skeleton/style.scss
Outdated
Show resolved
Hide resolved
Description
Resolves #27515
How has this been tested?
Screenshots
Types of changes
New feature
Checklist: