-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
List View: Design updates #24419
List View: Design updates #24419
Conversation
Size Change: -1.19 kB (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
Why is it a draft? It contains more than enough changes for an iteration 💯 |
I pushed some changes:
I'm sure it does. For now, this is kind of a sandbox to see how things feel in practice. It might be mergeable, or maybe we end up splitting it up. |
Sooo cool. Big Game Changer.... I was about to test it with http://gutenberg.run/24419, but unfortunately it does not work. I'm getting an error. I've tried it several times... @aduth |
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 working pretty well! Just a few comments below.
Also, if we're showing the "More options" menu (I'm not fully convinced we should 😅 ), we'll probably need to hide some of the options. E.g. trying to use "Move to" is awkward from the modal and impossible from the top toolbar navigator.
Finally:
- Lint checks are failing: run
npm run lint-js
once you've finished making changes and fix the resulting errors. - Treegrid snapshots need updating: run
npm run test-unit -- -u
to update them.
@@ -53,18 +51,15 @@ export default function BlockNavigationBlock( { | |||
const siblingCount = rowCount - 1; | |||
const hasSiblings = siblingCount > 0; | |||
const hasRenderedMovers = showBlockMovers && hasSiblings; | |||
|
|||
// I'm not using the classnames that come from this, | |||
// but maybe I should? -shaun |
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 would be good to keep the is-visible
class and attach the hover/focus styles to it, otherwise we're depending on focus-within
and that won't work on IE 😞
} | ||
|
||
// Hide block number and level description. (i.e. Block 2 of | ||
// 3, Level 2). This is mostly useful for screen readers. | ||
.block-editor-block-navigation-block-slot__description, | ||
.block-editor-block-navigation-block-select-button__description, | ||
.block-editor-block-navigation-appender__description { | ||
display: none; |
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.
We can't use display: none
if the content is meant to be screen reader accessible.
Current best practice in this repo is to wrap the screen-reader-only component in VisuallyHidden. This ensures it'll have the appropriate styling.
@@ -150,10 +150,10 @@ export default function TreeGrid( { children, ...props } ) { | |||
return ( | |||
<RovingTabIndexContainer> | |||
{ /* Disable reason: A treegrid is implemented using a table element. */ } |
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.
Need to update the disable reason here.
Here is the PR for WIP: Add drag and drop to List View |
Can we get an update to how things are going here? |
I was finally able to test this out by not using gutenberg.run. I followed this doc: https://developer.wordpress.org/block-editor/tutorials/devenv/ and I am following steps in how to test run a PR. I will later today add an issue and then create a doc PR giving instructions on how to run a PR. Here is an animated gif. Showing the Navigation screen structure. What is interesting is the clarity in when an element is selected. But the black background color creates a too strong contrast. The black could perhaps be a greyish color instead. As it might just need a much more subtitle background color. I am not sure about the dots. Having the old tree diagram lines seemed easier to follow. |
This allows the button to be bigger when nested. This also shows the more menu everywhere, makes movers appear side-by-side, and removes a bunch of unnecessary code from the descender component.
Basically deleted most of the current CSS and rewriting in a effort to simplify.
dab350f
to
d03445a
Compare
@shaunandrews is there anything specific we can do to help move this forward? |
I likely tried doing too much in this PR. I think we could probably break some of this up into smaller branches, like:
|
Cherry-picked a few things from this bigger PR into a smaller one over in #28029 |
@shaunandrews as the new screen has no list view anymore, is this design update maybe outside of the Navigation editor scope? |
I think we'll bring the list view back to the navigation editor, so it still makes sense to improve it as part of that effort. |
@draganescu @shaunandrews Should we remove this from |
WIP
Still a lot to do, but this PR starts by
There's still a lot of work to be done. Here's how the changes look in the experimental Navigation screen, which shows the movers: