Move filter UI into a toggle-able panel to improve experience on narrow viewports/containers #63203
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. |
|
Size Change: +725 B (+0.04%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
The button does suggest a drop-down. I also believe we had a dropdown design for filters somewhere, though there were likely complexity reasons for this. IMO this seems okay to me, as it improves the situation, but I'll defer to Jay or others! |
|
Thanks @jorgefilipecosta, this looks like a good start. One quick change would be to ensure the panel is closed by default, rather than open. We might also try opening by default when filters are applied, but I don't feel as strongly about that part. Given the visibility is now toggleable I think we can contain the filter UI within a dedicated row beneath That structural change would decrease the likelihood of wrapping on narrow containers. In other words this: 
Instead of this: 
|
jorgefilipecosta
force-pushed
the
update/Move-filter-UI-into-a-toggle-able-panel-to-improve-experience-on-narrow-viewports-containers
branch
from
e657486 to
1cd016e
Compare
|
Hi @jameskoster, the changes you suggested were implemented. |
|
It feels a bit weird that we have to click the toggle to then be presented with the Additionally should the |
|
There is one more nuance with |
Yes, I think that's a good point. When no filters are set the filter button could expose the 'Add filter' menu instead. Here's the flow:
Once filters are added it behaves like a toggle for the panel. |
jorgefilipecosta
force-pushed
the
update/Move-filter-UI-into-a-toggle-able-panel-to-improve-experience-on-narrow-viewports-containers
branch
from
8f603d2 to
87a5f74
Compare
Hi @ntsekouras, I think this issue is fixed.
Hi @jameskoster, the solution you suggested was implemented, let me know if it looks as expected. (the end to end tests are not updated yet, but if we agree on this solution I will do the update on this PR) |
packages/dataviews/src/dataviews.tsx
Outdated
| @@ -111,7 +119,7 @@ export default function DataViews< Item >( { | |||
| > | |||
| <HStack | |||
| justify="start" | |||
| className="dataviews-filters__container" | |||
| className="dataviews-search__container" | |||
There was a problem hiding this comment.
I guess we won't need the hstack now.. We need to verify though if it has visual side effects..
There was a problem hiding this comment.
Yes, we can remove hstack but I needed to add a small, a small CSS rule to make the search still expand.
The search input also got a little bit bigger but I think we have space so it may be ok to default to the normal input control width (240px). Going to cc: @WordPress/gutenberg-design to check if we can do this small dimension change.
|
This tests well and code wise looks good. Let's see the design input and see to update the tests afterwards. |
|
This is working really well! A couple of tiny visual tweaks:
|
jorgefilipecosta
force-pushed
the
update/Move-filter-UI-into-a-toggle-able-panel-to-improve-experience-on-narrow-viewports-containers
branch
from
6c19704 to
ff8f340
Compare
jorgefilipecosta
force-pushed
the
update/Move-filter-UI-into-a-toggle-able-panel-to-improve-experience-on-narrow-viewports-containers
branch
from
310e434 to
1eb6a6c
Compare
|
This PR passed by a big rebase, but it seems now everything is working again. |
|
Hi @jameskoster, thank you for the reviews, the first two tweaks you suggested were applied. The third one is being done on this PR #63755 as it is a change to a behavior already on the trunk. |
jorgefilipecosta
force-pushed
the
update/Move-filter-UI-into-a-toggle-able-panel-to-improve-experience-on-narrow-viewports-containers
branch
2 times, most recently
from
7e51dc2 to
0fa197e
Compare
|
Thanks for adding those tweaks :) I'm noticing a small bug; on the Pages table, adding one filter seems to add all filter options: filters.mp4 |
jorgefilipecosta
force-pushed
the
update/Move-filter-UI-into-a-toggle-able-panel-to-improve-experience-on-narrow-viewports-containers
branch
from
0fa197e to
f8efec1
Compare
jorgefilipecosta
force-pushed
the
update/Move-filter-UI-into-a-toggle-able-panel-to-improve-experience-on-narrow-viewports-containers
branch
from
cba4375 to
56affa9
Compare
|
@jorgefilipecosta I reckon we could move it right next to the search input with an 8px gap between: 
|
|
Hi @jameskoster the filter toggle/add button is now at the side of the search as suggested. |
jorgefilipecosta
force-pushed
the
update/Move-filter-UI-into-a-toggle-able-panel-to-improve-experience-on-narrow-viewports-containers
branch
from
1ab0349 to
226156e
Compare
|
I pushed a tiny style tweak to the count. There's only one small issue I see outstanding: The filter panel visibility persists as you navigate between different views. IE if I open the panel on one view, it stays open when I navigate to a different view. Ideally each view treats the open/close state dependently. Would that be possible? |
|
Hi @jameskoster thank you for the style enhancement.
I made an update, and now the state should not persist across the views. |
| @@ -49,6 +49,8 @@ type DataViewsProps< Item > = { | |||
| selection?: string[]; | |||
| onChangeSelection?: ( items: string[] ) => void; | |||
| header?: ReactNode; | |||
| isShowingFilter?: boolean; | |||
| setIsShowingFilter?: React.Dispatch< React.SetStateAction< boolean > >; | |||
There was a problem hiding this comment.
I would love if avoid these props please. I don't think we should be adding random props like that to the DataViews component.
Why do we need them?
There was a problem hiding this comment.
Hi @youknowriad, we added these props to reset the isShowingFilter state when the active view changes (e.g: from drafts, to scheduled). This allows us to control or reset the state from outside of the component. If these props aren't passed, the component will use its own state, but having them allows outside control when necessary.
There was a problem hiding this comment.
I see. I think it's not ideal in terms of Api for the DataViews component. Why control this state and not other states like dropdown states... I think this doesn't make for a great API in DataViews.
Two options from me:
- Can we accept that this remains an internal state.
- If it's about resetting the state, we can force remount with a separate
keyon DataViews if needed.
There was a problem hiding this comment.
If it's about resetting the state, we can force remount with a separate key on DataViews if needed.
I have thought of this option but I think it will probably also have undesired effects. I can give it a try and see how it goes.
jorgefilipecosta
force-pushed
the
update/Move-filter-UI-into-a-toggle-able-panel-to-improve-experience-on-narrow-viewports-containers
branch
from
211e6d8 to
617aa48
Compare
…ebug code). (+15 squashed commits) Squashed commits: [2198f16852] style change [0f23576ae6] hide filters when changing views [fbee4cad46] lint fix [eaaccae54b] end to end test updates [7b58faec1c] make filters toggle at the side of search [2f5a957a47] end to end test updates [9e40065f17] end to end test updare [9fd5ca03b5] lint filters and make the filter button the first [877f13bd54] Count styling [4fc07717be] Reduce spacing, remove negative margin [871ebf4553] Remove negative margin [0d8632a666] lint fix [31d893beb3] post rebase fixes [1d2b4bf1d3] fix don't show all filters [224d0ef7b1] fix number positioning (+3 squashed commits) Squashed commits: [f0acb40a7d] enhanements [309567963c] post rebase style fixes [9ce5f3686a] Remove hstack form search (+2 squashed commits) Squashed commits: [62c7124201] lint fixes [afa224734a] apply feedback
jorgefilipecosta
force-pushed
the
update/Move-filter-UI-into-a-toggle-able-panel-to-improve-experience-on-narrow-viewports-containers
branch
from
617aa48 to
8be7349
Compare
|
Hi @youknowriad, with #63889 merged and an additional small fix on search (included in this PR) the key-based solution to reset the filter showing state seems to be working. This PR is ready for review. |
There was a problem hiding this comment.
This is working well for me. What do you think @jameskoster
jorgefilipecosta
deleted the
update/Move-filter-UI-into-a-toggle-able-panel-to-improve-experience-on-narrow-viewports-containers
branch
|
@jameskoster I merged this one to avoid potential complex rebases as this PR has a considerable size. But if there is anything we should improve just let me know and I will gladly work on a follow-up. |
jorgefilipecosta
added
the
[Feature] DataViews
|
@jameskoster @jorgefilipecosta what are your thoughts regarding filters that declare to be primary?
There're some inconsistencies with regards to the +1 number as well. Note how it's not set in Patterns (sync is a "primary" filter) but it's set in Pages (all secondary filters) before the user picks a value: Gravacao.do.ecra.2024-08-19.as.17.13.30.movSo, I wonder if the concept of "primary filters" still make sense. If so, I believe we should make them visible as before. If we don't want to do that, I think we could just remove the |
|
Also note that the "Add filter" button is hidden if all filters are primary, and that creates weird interactions. Compare Patterns (only 1 filter that is primary) vs Templates (only 1 filter but it is NOT primary). It's not clear why the "Add filter" button would be visible but disabled in the later but hidden in the former: Gravacao.do.ecra.2024-08-19.as.17.23.42.mov |
We should fix that 👍 The question is whether to show the count when a filter is added, or when a value is selected. I can see arguments for both. What do you think? The intent in the original issue was for the panel to be open automatically if any filters are added. So if a view had primary filters the panel would be open. Looks like we missed tracking that as a follow-up.
I suspect this is a quirk relating the the a11y requirements. Most views supply non-primary filters that users can add as required. When all filters are added, the button remains accessible to help folks understand no more filters can be added, and to avoid confusion around the button vanishing from the UI. Because there's only a single filter on the Patterns page, and it's primary, users cannot ever add a new filter here. Consequently it's okay to hide the button entirely. |
|
Follow-up for having the filter toggle opened on load if there are primary filters #64651 |
|
There's another follow-up: what to do with the filter toggle when there are no filters? Report: #64631 There's this PR that hides the toggle #64640 Alternatively, we could show it as "disabled". However, I find the disabled state is useful when there's opportunities for the component to be active. In this case, given there are no filters, there're no opportunities for it to become active — hence my preference to just hide it. In the interest of velocity, I'll approve and go ahead with that PR. Let me know if you'd like to do something different. |
|
That seems reasonable to me 👍 |
|
There's also a couple of details about keyboard interaction / how the "add filter" button should work: |
Fixes: #60696
Adds a button to toggle the filter visibility. The button also shows the number of filters applied on the corner.
Screenshot
Testing Instructions
I verified a button to toggle the filter visibility was available and worked as expected.
I verified that the count shown on the button matched the number of filters enabled and if there are no filters, the count is not shown.