DataViews: extract FilterEnumeration component
#56514
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oandregal
requested review from
andrewhayward,
youknowriad,
ciampo,
jorgefilipecosta and
ntsekouras
oandregal
added
[Type] Experimental
This was referenced Nov 24, 2023
|
Size Change: -183 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
oandregal
commented
Nov 24, 2023
| @@ -55,7 +57,7 @@ const sortingItemsInfo = { | |||
| }; | |||
| const sortIcons = { asc: chevronUp, desc: chevronDown }; | |||
|
|
|||
| function HeaderMenu( { dataView, header } ) { | |||
| function HeaderMenu( { dataView, header, view, onChangeView } ) { | |||
There was a problem hiding this comment.
What do you all think of using both TanStack (dataView, header) and our own APIs (view, onChange) in HeaderMenu for the time being?
Refactoring view list to use our own APIs is a big PR. We can piece it down by temporarily allowing both things to coexist here and prepare small PRs that update it bit by bit (column filters, sorting, pagination, visibility, search, etc.).
There was a problem hiding this comment.
If this is not a plan we can agree upon, I'll remove 67d5dec from this PR and work on that separately,
There was a problem hiding this comment.
I'm ok with that personally, but let's add a task to follow-up on that and not leave it like that forever.
youknowriad
reviewed
Nov 24, 2023
| componentsPrivateApis | ||
| ); | ||
|
|
||
| export default function ( { filter, view, onChangeView } ) { |
There was a problem hiding this comment.
I like that we now have a FilterEnumeration component. I imagine in the future we could have a FilterString or something else as well. I also like to keep all of these components with similar API (props). The current props look good to me.
What I don't like is that this component assumes that it's rendered within a dropdown menu. For me, this makes the component less reusable and it's also not something that we will be able to support for all types of filters (how do you put an input as a dropdown menu item). I'd have preferred if this component was just a normal form.
There was a problem hiding this comment.
I'd have preferred if this component was just a normal form.
Could you clarify this bit? How would you do it?
I'd very much like having a standalone component that doesn't depend on the trigger component being any particular type. I'm just not sure how to achieve it with the existing component library. I've been resistant to open this PR until today, precisely because I didn't like this trigger/parent dependency.
There was a problem hiding this comment.
If I'm not wrong, dropdowns (not menu dropdowns) or modals can render any random component. So a design like something in my related comment here #56479 (comment) would be able to be achieved in a reusable component.
Pseudo code:
EnumarationFilter( props ) {
return (
<VStack>
<Select label="operator" values={ [ 'in', 'not in' ] } />
<Select label="value" multiple values={ props.filter.elements } />
</VStack>
);
}
There was a problem hiding this comment.
I see. Do you feel this PR would be useful towards implementing that vision (all components are centralized in a single place, so it's easier to change the UI for all of them at once)? Or do you feel we should clarify that vision first?
There was a problem hiding this comment.
I feel we should ship this one as it is and iterate on UI after. Everything (including updating UI) becomes easier.
There was a problem hiding this comment.
In my mind these intermediary steps are a detour and we should just build the generic filters. That said, I'm ok if we have different opinions here. So if you all agree about the current path, please go ahead. Nothing is set in stone.
ciampo
reviewed
Nov 30, 2023
|
|
||
| return filter.elements.map( ( element ) => { | ||
| return ( | ||
| <DropdownMenuCheckboxItem |
There was a problem hiding this comment.
As also discussed previously (and recently noted by @andrewhayward ), mutually exclusive options should be radio menu items, instead of checkbox menu items.
If we wanted to keep the scope of this PR focused on the refactor, we could make the changes to radios in a follow-up
There was a problem hiding this comment.
Thanks, I've addressed Andrew's feedback in that PR for filters and prepared a different one reviewing every other menu item in dataviews: #56676
|
I'm going to close this one and will revisit later, if necessary. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related #55083
What?
Extracts a new
FilterEnumerationcomponent for the existing filters:Why?
I plan to add new features to it, such as
NOT INoperator #56479 or multiselection #56468. Those are easier to implement if all the components that trigger the filter use the same component.How?
FilterEnumerationcomponent.AddFilterandFilterSummarycomponents, replacing the existing ones.Testing Instructions
TODO / Follow-ups
I'd like to also reuse this component in the column's filters. I'll do it in subsequent PRs, as that code uses TanStack APIs (I've gone ahead and update the viewlist here at 67d5dec. If it ends being too much or there is no agreement I'll extract to a different PR to work on separately.dataView/React Table andheader) instead of our ownview, etc. It needs a little refactoring first.