-
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 Filter search widget: do not use Composite store #64985
Dataviews Filter search widget: do not use Composite store #64985
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
e2e test failures seem related — will look into it |
Yeah, seems like we're not exporting |
@@ -84,22 +86,37 @@ const getNewValue = ( | |||
return [ value ]; | |||
}; | |||
|
|||
function generateCompositeItemId( | |||
prefix: string, | |||
filterElement: NormalizedFilter[ 'elements' ][ number ] |
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 directly pass the filter.elements[ 0 ].value
? Seems like only the value is necessary to construct the ID. Could potentially simplify the second argument type, too.
store={ compositeStore } | ||
virtualFocus | ||
focusLoop | ||
activeId={ activeCompositeId } |
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 were previously working with defaultActiveId
and not activeId
. Are we certain this won't introduce suble differences?
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 are using to activeId
instead of defaultActiveId
since we are switching from using Composite
uncontrolled, to controlling its active item ID state.
I moved the defaultActiveId
value to the initial state value for activeCompositeId
.
I'll look more into it, and report any findings
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 logic affects the composite widget when opening the dropdown — it basically prevents Composite
from automatically picking an active composite item if there is more than one filter operator.
I reviewed the code, and I don't think this change will introduce differences in the current state of the code since the filter.operators
array doesn't seem to change while the dialog is open.
If this assumption was ever going to change, I suspect the previous version would have also incurred in some unexpected behaviour
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.
In general, the current implementation feels a bit hacky. I wish we didn't have to control the activeId
state at all. With more time and and a willingness to tweak this UI a bit, we could also consider having different styles for active vs focused items.
But not in the scope for this PR, and without a high priority.
7c7975b
to
0dc1866
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.
Tests well and code LGTM, thank you 🚀
What?
Extracted from #64723
Refactor the dataviews filter search widget so that it doesn't use the
store
fromuseCompositeStore
to function.Why?
See #63704 (comment)
How?
By controlling the
activeId
state via props, and reacting to theonFocusVisible
callbackTesting Instructions
There shouldn't be any noticeably different behaviors — make sure that the following happens both in
trunk
and in this PR:Screenshots
Kapture.2024-09-02.at.18.09.44.mp4