-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Navigation: Implement redesign of Navigation Editor #25178
Conversation
Size Change: -1.17 kB (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
packages/edit-navigation/src/components/header/use-menu-locations.js
Outdated
Show resolved
Hide resolved
packages/edit-navigation/src/components/header/use-menu-locations.js
Outdated
Show resolved
Hide resolved
packages/edit-navigation/src/components/header/use-menu-locations.js
Outdated
Show resolved
Hide resolved
This looks really nice! |
@noisysocks There was also a discussion about adding a label to the menu select, but not sure if a firm decision had been made on that - #24875 (comment) |
Whoops—my bad. Fixed in e414e3f. Try again now! |
@shaunandrews: How should this look when there are no menus? This would happen, for example, when the user is setting up a brand new site. We could automatically drop the user into the Add new flow but I worry that this doesn't make it clear to the user what they need to do. Also, the Add new and Cancel buttons wouldn't do anything useful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The design looks very good on desktop, but on mobile it's a bit weird to have the list view between the block toolbar and the blocks themselves.
The other side of this is the changed markup order has disrupted keyboard navigation: it should be possible to Shift+Tab from the block to its toolbar (just like in the post editor), and with this change Shift+Tab brings us to the list view first, and then to end of the toolbar where the block inspector toggle is.
Apart from that, only a couple of comments below 😅
Thanks for testing @tellthemachines! I made things look a little nicer on mobile in cb544c4 by splitting the toolbar into two rows when the viewport is small. We can't make the tab order @shaunandrews: How do you feel about moving the List view to the right of the screen? |
Co-authored-by: Kai Hao <kevin830726@gmail.com>
Co-authored-by: Kai Hao <kevin830726@gmail.com>
cb544c4
to
86440fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is getting pretty close! I left a few comments.
One thing I noticed is we still have the color options on the Nav block toolbar. Shouldn't we get rid of those for this screen?
flex-direction: column; | ||
|
||
.block-editor-block-card { | ||
order: -2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oooh we shouldn't do this, it messes up keyboard focus order.
I don't think there's a way to add items to the inspector in any position, because the slot is positioned in a specific place; perhaps the component could be changed to include another slot, but... do we really need the top level pages checkbox to be this prominent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is a total hack. We need to come up with a better way of doing this. Maybe we could add an order
prop to InspectorControls
. I've made a note to come back to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good!
I'm not sure about the useSelect
second argument though, feel free to dismiss them if they don't make sense!
|
||
export default function ListView( { isPending, blocks } ) { | ||
const [ selectedBlockId, setSelectedBlockId ] = useState( | ||
blocks[ 0 ]?.clientId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when blocks
changes?
For example, it changes from [1, 2, 3]
to [4, 5, 6]
. selectedBlockId
is now out of sync, should we care about this at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest I'm not even sure why we're using local state instead of select( 'core/block-editor' ).getSelectedBlockClientId()
. Maybe @talldan knows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was already this way in master
, so I've made a note to come back to this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages/edit-navigation/src/components/header/manage-locations.js
Outdated
Show resolved
Hide resolved
const [ menuLocationsByName, setMenuLocationsByName ] = useState( null ); | ||
|
||
useEffect( () => { | ||
let isMounted = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wrong, this is not ideal 😓 , see https://reactjs.org/blog/2015/12/16/ismounted-antipattern.html
However, it's the best we can get, and it doesn't really matter that much in this context. Just want to note that, maybe we should add an API in apiFetch
to allow canceling/aborting the request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to note that, maybe we should add an API in apiFetch to allow canceling/aborting the request.
Good idea.
packages/edit-navigation/src/components/inspector-additions/auto-add-pages-panel.js
Outdated
Show resolved
Hide resolved
const [ autoAddPages, setAutoAddPages ] = useState( null ); | ||
|
||
useEffect( () => { | ||
if ( autoAddPages === null && menu ) { | ||
setAutoAddPages( menu.auto_add ); | ||
} | ||
}, [ autoAddPages, menu ] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just set initialState to menu.auto_add
?
const [ autoAddPages, setAutoAddPages ] = useState( null ); | |
useEffect( () => { | |
if ( autoAddPages === null && menu ) { | |
setAutoAddPages( menu.auto_add ); | |
} | |
}, [ autoAddPages, menu ] ); | |
const [ autoAddPages, setAutoAddPages ] = useState( menu.auto_add ); |
Or better, just derive state from props.
const [ autoAddPages, setAutoAddPages ] = useState( null ); | |
useEffect( () => { | |
if ( autoAddPages === null && menu ) { | |
setAutoAddPages( menu.auto_add ); | |
} | |
}, [ autoAddPages, menu ] ); | |
const autoAddPages = menu.auto_add; |
Since saveMenu
updates menu
anyway, what's the benefit of introducing a new state here?
Do we want it to always defaults to false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The local state is so that the checkbox below can be a controlled component. We can't initialise this state to menu.auto_add
because menu
is null
when the component mounts and when the REST API request is pending.
We could remove the useSelect()
by passing menu
as a prop instead of menuId
but it doesn't change much because the menu
prop is still potentially null
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind me. I see what you mean now. Yes, we should be able to rely on @wordpress/data
to persist this state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. Annoyingly, when I do this, getMenus()
starts a GET /menus
request immediately after saveMenu()
is dispatched. This clobbers the auto_add
value that we just saved because the backend hasn't had a chance to persist the new value to the database.
I'll need to dig into why @wordpress/data
does this and fix it there. It should be smart enough to not issue unnecessary API requests. I've made a note to come back to this later.
onClick={ () => { | ||
if ( | ||
// eslint-disable-next-line no-alert | ||
window.confirm( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be using window.confirm
? If this is temporary code only, maybe we should add a FIXME
comment above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I didn't know #16583 exists, thanks!
packages/edit-navigation/src/components/inspector-additions/index.js
Outdated
Show resolved
Hide resolved
packages/edit-navigation/src/components/layout/use-menu-notifications.js
Outdated
Show resolved
Hide resolved
packages/edit-navigation/src/components/layout/use-navigation-editor.js
Outdated
Show resolved
Hide resolved
@tellthemachines: 1b668fb should fix that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Late to the party but I'd definitely second this. There should be better affordance across all the new UIs and wherever theres a toolbar, one single Shift+Tab should take from the edited object to its toolbar. For example, right now when I select part of a menu item because I want to make it bold:
Will create a new issue. |
Implements the broad strokes of the Navigation Editor redesign described in #24875.
Closes #25010. Closes #25014.
I focused mainly on layout and component structure changes. The smaller details described in #24875 can be implemented in smaller follow-up PRs. Ultimately this turned into a near-rewrite of the screen.
What's in this PR
What's NOT in this PR
Tasks to do after merge
InspectorControls
. See Navigation: Implement redesign of Navigation Editor #25178 (comment).getSelectedBlockClientId()
? See Navigation: Implement redesign of Navigation Editor #25178 (comment).@wordpress/data
issue. See Navigation: Implement redesign of Navigation Editor #25178 (comment).