Skip to content

Fix: DataViews modal actions in list layout #72793

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
merged 3 commits into from
Oct 29, 2025
Merged

Fix: DataViews modal actions in list layout #72793

merged 3 commits into from
Oct 29, 2025
+10 −4

Conversation

@oandregal
Copy link
Member

@oandregal oandregal commented Oct 29, 2025

What?

This PR fixes an issue by which modal actions in list layout wouldn't trigger the modal.

How?

Use the text property of the button to render text instead of using children.

Testing Instructions

  • Open the storybook, go to the the story "DataViews > Default".
  • Switch to list layout.
  • Click on the delete action.

The expectation is that it opens the delete modal.

@oandregal oandregal self-assigned this Oct 29, 2025
@github-actions
Copy link

github-actions bot commented Oct 29, 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: oandregal <oandregal@git.wordpress.org>
Co-authored-by: jorgefilipecosta <jorgefilipecosta@git.wordpress.org>

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

@oandregal oandregal 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 labels Oct 29, 2025
oandregal
oandregal commented Oct 29, 2025
View reviewed changes
packages/dataviews/src/stories/dataviews.fixtures.tsx
id: 'secondary',
label: 'Secondary action',
callback() {},
callback() {
Copy link
Member Author

@oandregal oandregal Oct 29, 2025

Choose a reason for hiding this comment

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

Unrelated to the issue: just thought I'd add it so it's easier for us to track this.

jorgefilipecosta
jorgefilipecosta approved these changes Oct 29, 2025
View reviewed changes
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Worked well on my tests, although we still need to improve the UI on list view.

@oandregal oandregal added the Backport to WP 6.9 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 29, 2025
oandregal
oandregal commented Oct 29, 2025
View reviewed changes
packages/dataviews/src/dataviews-layouts/list/index.tsx
>
{ label }
</Button>
/>
Copy link
Member Author

@oandregal oandregal Oct 29, 2025

Choose a reason for hiding this comment

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

It's not clear to me why a button with children would prevent the modal from triggering. We need to investigate more the CompositeItem render behavior, though it's good to merge it anyway.

Copy link
Member

@tyxla tyxla Oct 31, 2025

Choose a reason for hiding this comment

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

@WordPress/gutenberg-components this looks like an interesting one

Copy link
Member

@aduth aduth Nov 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think the issue is that you're passing children to both the Composite.Item and the render element's Button.

This behavior is described in https://ariakit.org/guide/composition (emphasis mine):

This also applies to the children prop. You don't need to nest children within the render prop. But if you do, the children passed to the rendered element will override the original component children.

So when you pass children to Button, the content below { isModalOpen && /* ... */ } will be overridden, which is what prevents the modal from appearing.

A fixed version looks something like...

<Composite.Item
    /* ... */
	render={
		<Button /* ... */>
			{ label }
			{ isModalOpen && ( /* ... */ ) }
		</Button>
	}
/>

(or conversely, shifting all of Button's content to be a children of the Composite.Item)

Copy link
Member

@aduth aduth Nov 11, 2025

Choose a reason for hiding this comment

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

This is potentially something we could lint for, i.e. passing children to both a render prop element and the component itself is likely to produce an unexpected result.

@oandregal oandregal force-pushed the fix/list-primary-actions-click branch from 644164d to 5daa7c4 Compare October 29, 2025 16:57
@oandregal oandregal enabled auto-merge (squash) October 29, 2025 16:57
@github-actions
Copy link

github-actions bot commented Oct 29, 2025

Flaky tests detected in 5daa7c4.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/18915879597
📝 Reported issues:

@oandregal oandregal merged commit 32d161d into trunk Oct 29, 2025
34 checks passed
@oandregal oandregal deleted the fix/list-primary-actions-click branch October 29, 2025 17:35
@github-actions github-actions bot added this to the Gutenberg 22.1 milestone Oct 29, 2025
@github-actions
Copy link

github-actions bot commented Oct 29, 2025

There was a conflict while trying to cherry-pick the commit to the wp/6.9 branch. Please resolve the conflict manually and create a PR to the wp/6.9 branch.

PRs to wp/6.9 are similar to PRs to trunk, but you should base your PR on the wp/6.9 branch instead of trunk.

# Checkout the wp/6.9 branch instead of trunk.
git checkout wp/6.9
# Create a new branch for your PR.
git checkout -b my-branch
# Cherry-pick the commit.
git cherry-pick 32d161df6c2d12554bca0bccb842f16c95ce6950
# Check which files have conflicts.
git status
# Resolve the conflict...
# Add the resolved files to the staging area.
git status
git add .
git cherry-pick --continue
# Push the branch to the repository
git push origin my-branch
# Create a PR and set the base to the wp/6.9 branch.
# See https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-base-branch-of-a-pull-request.

oandregal added a commit that referenced this pull request Oct 29, 2025
@oandregal @jorgefilipecosta
Fix: DataViews modal actions in list layout (#72793)
e5d3489 
Co-authored-by: oandregal <oandregal@git.wordpress.org>
Co-authored-by: jorgefilipecosta <jorgefilipecosta@git.wordpress.org>
@oandregal oandregal mentioned this pull request Oct 29, 2025
Merged
@oandregal
Copy link
Member Author

oandregal commented Oct 29, 2025

Backport at #72804

oandregal added a commit that referenced this pull request Oct 29, 2025
@oandregal @jorgefilipecosta
Fix: DataViews modal actions in list layout (#72793) (#72804)
bc9d82c 
Co-authored-by: jorgefilipecosta <jorgefilipecosta@git.wordpress.org>
@oandregal oandregal removed the Backport to WP 6.9 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 29, 2025
@priethor priethor added the Backport to WP 6.9 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Nov 7, 2025
@github-actions
Copy link

github-actions bot commented Nov 7, 2025

There was a conflict while trying to cherry-pick the commit to the wp/6.9 branch. Please resolve the conflict manually and create a PR to the wp/6.9 branch.

PRs to wp/6.9 are similar to PRs to trunk, but you should base your PR on the wp/6.9 branch instead of trunk.

# Checkout the wp/6.9 branch instead of trunk.
git checkout wp/6.9
# Create a new branch for your PR.
git checkout -b my-branch
# Cherry-pick the commit.
git cherry-pick 32d161df6c2d12554bca0bccb842f16c95ce6950
# Check which files have conflicts.
git status
# Resolve the conflict...
# Add the resolved files to the staging area.
git status
git add .
git cherry-pick --continue
# Push the branch to the repository
git push origin my-branch
# Create a PR and set the base to the wp/6.9 branch.
# See https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-base-branch-of-a-pull-request.

@priethor priethor added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Backport to WP 6.9 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Nov 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduth aduth aduth left review comments

@tyxla tyxla tyxla left review comments

@jorgefilipecosta jorgefilipecosta jorgefilipecosta approved these changes

@mikejolley mikejolley Awaiting requested review from mikejolley

Assignees

@oandregal oandregal

Labels

Backported to WP Core Pull request that has been successfully merged into WP Core [Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond [Type] Bug An existing feature does not function as intended

Projects

None yet

Milestone

Gutenberg 22.1

Development

Successfully merging this pull request may close these issues.

6 participants

@oandregal @aduth @tyxla @jorgefilipecosta @priethor