-
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
DataViews: update filter component #56110
Conversation
Size Change: -19 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
a936000
to
141d4a4
Compare
Ready to receive feedback here. It's almost there, though there are some issues to fix (see |
Nice! The dropdown menu is working well. In terms of a11y buttons should generally describe what will happen on click which isn't how this is working currently. I wonder if a label/tooltip that says something like "Change status filter" would be satisfactory. It'd be good to get @andrewhayward's thoughts on this. Appreciate the design is still being worked on but noticed a detail that could be easily overlooked: As the filter button widths increase the chevron icon seems to shrink: I know this is most likely follow-up territory but leaving a note anyway; let's update the status filter operator so that you can select multiple statuses. Let me know if I should open an issue for that. |
Yeah, I've noticed that too. This also happens in Gravacao.do.ecra.2023-11-16.as.13.19.32.movI'm going to ping @ciampo for thoughts. I've tried setting the |
This is a tricky one, with no clear and obvious solution. As @jameskoster points out, buttons should ideally have a name that indicates their purpose. We could use <button aria-label="Author" ...>Author is Person</button> ...but that brings its own issues, as ideally the visible label and accessible name should match. We could consider making these comboboxes, which have a separate label and value, and are probably more appropriate for what the button does... <button aria-label="Author" role="combobox" ...>Person</button> ...but again, we'd have labelling issues, particularly because we would no longer have a visible label. We could go back to something similar to the first iteration... <span class="wrapper">
<label for="combobox">Author</label> is
<button id="combobox" role="combobox" ...>Person</button>
</span> ...styling the Definitely needs to experimentation and testing. |
I have a feeling this issue is linked to these gutenberg/packages/components/src/flex/styles.ts Lines 40 to 44 in cf1a1da
gutenberg/packages/components/src/button/style.scss Lines 283 to 289 in cf1a1da
If we remove/override those, things start to look a little more coherent... |
I think it would be good to check what other tools are doing, this pattern is common. For instance what's the story of these controls in notion or GitHub. |
This is the issue yes. svgs need to have a Regarding the other spacing issues, I noticed that we don't need to wrap |
const { DropdownMenuV2, DropdownMenuItemV2, DropdownMenuSeparatorV2 } = unlock( | ||
componentsPrivateApis | ||
); |
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.
Could you try to use the DrodpdownMenuV2Ariakit
component? The DropdownMenuV2
component is likely going to removed soon.
(also, these names are all temporary, it will improve once I'll delete DropdownMenuV2
)
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 tried, and left some comments at #56110 (comment)
activeElement.label | ||
) | ||
: filter.name } | ||
<Icon icon={ chevronDown } /> |
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.
Have you tried using the icon
prop on the Button
component? Would that make any difference?
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.
Something like this could work:
<Button icon={chevronDown} iconPosition="right" text={...} />
Although:
- there seems to be a bug in the button component in recognizing where the button is icon-only or has text too
- I don't like the "right" keyword ("inline-end" would be better)
- in general, I prefer composition using
children
So, I think that the current approach is better.
<DropdownMenuItemV2 | ||
key={ element.value } | ||
suffix={ | ||
activeElement?.value === element.value && ( | ||
<Icon icon={ check } /> | ||
) | ||
} |
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 are dedicated dropdown menu items for checkboxes and radio, which already take care of semantics and check icons — see these two examples for the new DropdownMenuV2Ariakit
component:
And related code:
gutenberg/packages/components/src/dropdown-menu-v2-ariakit/stories/index.story.tsx
Lines 149 to 307 in 36617fb
export const WithCheckboxes: StoryFn< typeof DropdownMenu > = ( props ) => { | |
const [ isAChecked, setAChecked ] = useState( false ); | |
const [ isBChecked, setBChecked ] = useState( true ); | |
const [ multipleCheckboxesValue, setMultipleCheckboxesValue ] = useState< | |
string[] | |
>( [ 'b' ] ); | |
const onMultipleCheckboxesCheckedChange: React.ComponentProps< | |
typeof DropdownMenuCheckboxItem | |
>[ 'onChange' ] = ( e ) => { | |
setMultipleCheckboxesValue( ( prevValues ) => { | |
if ( prevValues.includes( e.target.value ) ) { | |
return prevValues.filter( ( val ) => val !== e.target.value ); | |
} | |
return [ ...prevValues, e.target.value ]; | |
} ); | |
}; | |
return ( | |
<DropdownMenu { ...props }> | |
<DropdownMenuGroup> | |
<DropdownMenuGroupLabel> | |
Individual, uncontrolled checkboxes | |
</DropdownMenuGroupLabel> | |
<DropdownMenuCheckboxItem | |
name="checkbox-individual-uncontrolled-a" | |
value="a" | |
> | |
Checkbox item A (initially unchecked) | |
</DropdownMenuCheckboxItem> | |
<DropdownMenuCheckboxItem | |
name="checkbox-individual-uncontrolled-b" | |
value="b" | |
defaultChecked | |
> | |
Checkbox item B (initially checked) | |
</DropdownMenuCheckboxItem> | |
</DropdownMenuGroup> | |
<DropdownMenuSeparator /> | |
<DropdownMenuGroup> | |
<DropdownMenuGroupLabel> | |
Individual, controlled checkboxes | |
</DropdownMenuGroupLabel> | |
<DropdownMenuCheckboxItem | |
name="checkbox-individual-controlled-a" | |
value="a" | |
checked={ isAChecked } | |
onChange={ ( e ) => setAChecked( e.target.checked ) } | |
> | |
Checkbox item A | |
</DropdownMenuCheckboxItem> | |
<DropdownMenuCheckboxItem | |
name="checkbox-individual-controlled-b" | |
value="b" | |
checked={ isBChecked } | |
onChange={ ( e ) => setBChecked( e.target.checked ) } | |
> | |
Checkbox item B (initially checked) | |
</DropdownMenuCheckboxItem> | |
</DropdownMenuGroup> | |
<DropdownMenuSeparator /> | |
<DropdownMenuGroup> | |
<DropdownMenuGroupLabel> | |
Multiple, uncontrolled checkboxes | |
</DropdownMenuGroupLabel> | |
<DropdownMenuCheckboxItem | |
name="checkbox-multiple-uncontrolled" | |
value="a" | |
> | |
Checkbox item A (initially unchecked) | |
</DropdownMenuCheckboxItem> | |
<DropdownMenuCheckboxItem | |
name="checkbox-multiple-uncontrolled" | |
value="b" | |
defaultChecked | |
> | |
Checkbox item B (initially checked) | |
</DropdownMenuCheckboxItem> | |
</DropdownMenuGroup> | |
<DropdownMenuSeparator /> | |
<DropdownMenuGroup> | |
<DropdownMenuGroupLabel> | |
Multiple, controlled checkboxes | |
</DropdownMenuGroupLabel> | |
<DropdownMenuCheckboxItem | |
name="checkbox-multiple-controlled" | |
value="a" | |
checked={ multipleCheckboxesValue.includes( 'a' ) } | |
onChange={ onMultipleCheckboxesCheckedChange } | |
> | |
Checkbox item A (initially unchecked) | |
</DropdownMenuCheckboxItem> | |
<DropdownMenuCheckboxItem | |
name="checkbox-multiple-controlled" | |
value="b" | |
checked={ multipleCheckboxesValue.includes( 'b' ) } | |
onChange={ onMultipleCheckboxesCheckedChange } | |
> | |
Checkbox item B (initially checked) | |
</DropdownMenuCheckboxItem> | |
</DropdownMenuGroup> | |
</DropdownMenu> | |
); | |
}; | |
WithCheckboxes.args = { | |
...Default.args, | |
}; | |
export const WithRadios: StoryFn< typeof DropdownMenu > = ( props ) => { | |
const [ radioValue, setRadioValue ] = useState( 'two' ); | |
const onRadioChange: React.ComponentProps< | |
typeof DropdownMenuRadioItem | |
>[ 'onChange' ] = ( e ) => setRadioValue( e.target.value ); | |
return ( | |
<DropdownMenu { ...props }> | |
<DropdownMenuGroup> | |
<DropdownMenuGroupLabel> | |
Uncontrolled radios | |
</DropdownMenuGroupLabel> | |
<DropdownMenuRadioItem name="radio-uncontrolled" value="one"> | |
Radio item 1 | |
</DropdownMenuRadioItem> | |
<DropdownMenuRadioItem | |
name="radio-uncontrolled" | |
value="two" | |
defaultChecked | |
> | |
Radio item 2 (initially checked) | |
</DropdownMenuRadioItem> | |
</DropdownMenuGroup> | |
<DropdownMenuSeparator /> | |
<DropdownMenuGroup> | |
<DropdownMenuGroupLabel> | |
Controlled radios | |
</DropdownMenuGroupLabel> | |
<DropdownMenuRadioItem | |
name="radio-controlled" | |
value="one" | |
checked={ radioValue === 'one' } | |
onChange={ onRadioChange } | |
> | |
Radio item 1 | |
</DropdownMenuRadioItem> | |
<DropdownMenuRadioItem | |
name="radio-controlled" | |
value="two" | |
checked={ radioValue === 'two' } | |
onChange={ onRadioChange } | |
> | |
Radio item 2 (initially checked) | |
</DropdownMenuRadioItem> | |
</DropdownMenuGroup> | |
</DropdownMenu> | |
); | |
}; | |
WithRadios.args = { | |
...Default.args, | |
}; |
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 switched to DropdownMenuCheckboxItem
at 0bc02b0, thanks.
Tried the new Ariakit ones (203fb3f and then reverted 6649de3):
- the radio doesn't allow having a "non-checked" state
- the checkbox performs the same as the current one, though there's a subtle style different when the element is focused, so I think it's best to update all existing dropdowns at once (I can help with this):
V2 | V2Ariakit |
---|---|
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.
Thank you for the feedback!
the radio doesn't allow having a "non-checked" state
Regarding the radios, could be more specific?
- If you mean that the component can't be initially rendered with no selection, that is probably a bug
- If, instead, you mean that once a radio item is selected, it's not possible to "un-select" it, then that's by design (see this comment for more info). The correct approach is then to either switch to checkboxes (in case more than one item can be selected at the same time), or to add a "None" (or any other label) additional menu item.
the checkbox performs the same as the current one, though there's a subtle style different when the element is focused, so I think it's best to update all existing dropdowns at once
Makes sense. FYI I've been working on applying the latest design feedback to the ariakit
version (the radix version is no longer maintained and scheduled for removal as soon as all usages are migrated)
(I can help with this)
Thank you!
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.
If, instead, you mean that once a radio item is selected, it's not possible to "un-select" it, then that's by design (see this comment for more #55625 (comment)). The correct approach is then to either switch to checkboxes (in case more than one item can be selected at the same time), or to add a "None" (or any other label) additional menu item.
It was this. Thanks for clarifying how it works. I've lost track in which PR/issue I saw this mockup, but I saw something like this:
- filters that allow multiselection would use a check
- filters that only allow a single item being selected would use a radio
Radios working as you mentioned, we need to clarify the expectations for them. I'm now working on adding multi-selection support at #56468 so I'll ask design there (will cc you for awareness, but no need to respond!).
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.
follow-up #56468 (comment)
I see a few things happening:
You should be able to set the |
This reverts commit 203fb3f.
4210db3
to
a18dd74
Compare
Fixed the shrinking icons for filters at 98f88e5 using Marko's suggestion ( |
To control scope and ship this faster, I'd like to work on the following features as follow-ups:
Considering those follow-ups, what's missing in this PR to be merged? Gravacao.do.ecra.2023-11-20.as.14.52.00.mov |
In terms of design we're looking good, but I'd consider multi-selection and a11y improvements as high-prio follow-ups. |
Yeah 👍 |
ok. The advantage of this landing in the current state is that those features can be parallelized: @andrewhayward can help with the labels issue while I work on multiselection, for example :) For the record, I see multiselection and "not in" filter as high priority, as they affect the API: we may want to create a dataviews package for people to tinker with, and I'd like the API as settled as possible by then. |
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.
Happy to approve for design based on that plan.
Part of #55083
Related #55100
What?
Updates the filter component to use a
Dropdown
instead of aSelectControl
.Gravacao.do.ecra.2023-11-20.as.14.52.00.mov
Why?
Implement the design mockups.
How?
Updates the
InFilter
component to use a Dropdown.Testing Instructions
Follow-ups