Skip to content

DataViews: open the filters from the column table consistently #72998

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.

Sign up for GitHub

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

Jump to bottom
Merged
oandregal merged 6 commits into from
Nov 21, 2025
Merged

DataViews: open the filters from the column table consistently #72998

oandregal merged 6 commits into from
Nov 21, 2025

Conversation

@hbhalodia
Copy link
Contributor

@hbhalodia hbhalodia commented Nov 5, 2025

What?

Closes #72986

Why?

  • Fix the inconsistent behavior for the DataViews filter.

How?

  • Add the useEffect to check for the filters and update the state as needed.

Testing Instructions

  1. Navigate to storybook,
  2. On page load, without any filter applied, click the "Categories" column and "Add filter".
  3. Notice it would open now.
  4. Remove that filter
  5. Repeat step Add/local build #2.
  6. Notice it would open again, so this is a consistent behavior.

Testing Instructions for Keyboard

  • None

Screenshots or screencast

Screen.Recording.2025-11-05.at.4.06.44.PM.mov

@hbhalodia hbhalodia self-assigned this Nov 5, 2025
@hbhalodia hbhalodia requested a review from oandregal as a code owner November 5, 2025 10:37
@hbhalodia hbhalodia added [Type] Bug An existing feature does not function as intended [Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond [Package] DataViews /packages/dataviews labels Nov 5, 2025
@hbhalodia hbhalodia mentioned this pull request Nov 5, 2025
Closed
6 tasks
@github-actions
Copy link

github-actions bot commented Nov 5, 2025 •
edited
Loading

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: hbhalodia <[email protected]>
Co-authored-by: StevenDufresne <[email protected]>
Co-authored-by: ntsekouras <[email protected]>
Co-authored-by: oandregal <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

StevenDufresne
StevenDufresne reviewed Nov 10, 2025
View reviewed changes
packages/dataviews/src/components/dataviews/index.tsx Outdated
}, [ hasPrimaryOrLockedFilters, isShowingFilter ] );

// Show filter panel when filters are added (but allow manual hiding).
const [ prevFiltersLength, setPrevFiltersLength ] = useState(
Copy link
Contributor

@StevenDufresne StevenDufresne Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My instinct is that using a ref would be safer and more performant, seeing that setPrevFiltersLength won't be used in a different context. We can avoid rerenders and having prevFiltersLength in the dependency array, and then updating its value opens the door for infinite loops.

Copy link
Contributor Author

@hbhalodia hbhalodia Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now updated.

ntsekouras
ntsekouras reviewed Nov 10, 2025
View reviewed changes
packages/dataviews/src/components/dataviews/index.tsx Outdated
currentFiltersLength > prevFiltersLengthRef.current &&
! isShowingFilter
) {
setIsShowingFilter( true );
Copy link
Contributor

@ntsekouras ntsekouras Nov 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if a better fix would be to just call this in table column header's add filter here. WDYT @oandregal ?

Copy link
Contributor Author

@hbhalodia hbhalodia Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ntsekouras, I tried that but setting explicitly there I thought was not better, even in filter's if you log the isShowingFilter, it is correctly showing the state, but only on initial filter check even it logs true, still it does not opens the filter panel.

Copy link
Member

@oandregal oandregal Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, updating the state in the table layout sounds better. Was there any issue with that approach, @hbhalodia ? I see the setIsShowingFilter function is already in the context, so available in the layout as well.

Copy link
Contributor Author

@hbhalodia hbhalodia Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue was not something with approach, We can set that explicitly in the column header, but my concern was, it was not working on first refresh, but thereafter it was working as expected, so updating the state in column header component wouldn't be a solution just to add a fix for this and explicitly adding to that component.

I can update the PR with tha approach (it was my first approach) but then thought of this, hence changed to that.

Updating the PR with the setting state via column header.

Copy link
Contributor Author

@hbhalodia hbhalodia Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @oandregal, This is now updated, to use context within column-header.

Copy link
Contributor Author

@hbhalodia hbhalodia Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @oandregal @ntsekouras, Can you please take a look on PR. Updated it with the required changes.

Thank You 🙇,

@hbhalodia hbhalodia requested a review from gigitux as a code owner November 18, 2025 09:01
@hbhalodia hbhalodia requested a review from ntsekouras November 21, 2025 07:25
@oandregal oandregal changed the title Fix: Clicking "Add filter" doesn't consistently open the filters for you to select a value - #72986 DataViews: open the filters from the column table consistently Nov 21, 2025
@oandregal
Copy link
Member

oandregal commented Nov 21, 2025

Thanks for working on this @hbhalodia

@oandregal oandregal merged commit c84546e into WordPress:trunk Nov 21, 2025
38 of 39 checks passed
@github-actions github-actions bot added this to the Gutenberg 22.2 milestone Nov 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StevenDufresne StevenDufresne StevenDufresne left review comments

@oandregal oandregal oandregal approved these changes

@gigitux gigitux Awaiting requested review from gigitux gigitux is a code owner

@ntsekouras ntsekouras Awaiting requested review from ntsekouras ntsekouras is a code owner

Assignees

@hbhalodia hbhalodia

Labels

[Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond [Package] DataViews /packages/dataviews [Type] Bug An existing feature does not function as intended

Projects

None yet

Milestone

Gutenberg 22.2

Development

Successfully merging this pull request may close these issues.

Clicking "Add filter" doesn't consistently open the filters for you to select a value.

4 participants

@hbhalodia @oandregal @StevenDufresne @ntsekouras