Size Change: +1 B (0%)

Total Size: 2.37 MB

_{compressed-size-action}

Looks good at a glance. I do wonder, since this toggle exists both here and in Preferences, can we move it away from the fixed position footer, and into the flow of the patterns? It can be top right of the content if it needs visibility, but then we'd probably need a subheading on the left to balance it out. Otherwise at the bottom, but you'd have to scroll to find it.

An alternative is to minimize the padding of the bottom reserved space, but it's unclear the componentry allows that without CSS overrides, which we should avoid.

For me the motivation here isn't to hide it, but to optimize the available space for patterns. What do you think? Absent that, if you want to keep the PR small, this can work as a first step.

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: juanfra <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: jasmussen <[email protected]>
Co-authored-by: karmatosed <[email protected]>
Co-authored-by: jameskoster <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: hanneslsm <[email protected]>

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

Thanks for taking a look, Joen!

Looks good at a glance. I do wonder, since this toggle exists both here and in Preferences, can we move it away from the fixed position footer, and into the flow of the patterns?

I hear you on optimizing the space for patterns. My only hesitation with moving the checkbox into the pattern flow is discoverability. A big part of why this toggle landed here in the first place was that many folks weren’t aware of the toggle in Preferences, and the modal was causing some friction. Keeping the checkbox visible helps address that.

If we place it inside the flow, I worry we’d slip back into the original problem. Putting it at the top could work, though it may feel a bit unbalanced.

We could also explore reducing the height of the fixed footer so it feels less heavy. Happy to iterate in that direction if it makes sense.

We could also explore reducing the height of the fixed footer so it feels less heavy. Happy to iterate in that direction if it makes sense.

Yep, that's my second suggestion. If there is a clean way to adjust the paddings of the persistent modal footer, that seems the way to go. But don't do it unless it can cleanly be done with the componentry, i.e. don't write custom CSS to make it happen. If there's no clean way to do it, we land this as is—though perhaps I'd still right-align the toggle.

This has design feedback and a way forward, so let’s remove that label and get some focus.

I reckon this is okay as is, to be honest. The positioning seems fairly standard for these kinds of checkboxes.

It would be good to refactor this modal using DataViewsPicker at some point.

Thank you all for the input and taking a look!

If there is a clean way to adjust the paddings of the persistent modal footer, that seems the way to go. But don't do it unless it can cleanly be done with the componentry, i.e. don't write custom CSS to make it happen.

@jasmussen the height is adjusted here so we shouldn't have to work on any new implementation to change that. I updated it to 72px to match what we have in the header, which helps keep things visually balanced.

Here's how it looks with the checkbox aligned to the right. My preference is to keep it on the left (for the reasons noted in the issue), though I realize that can be subjective.

If there's agreement I believe we could get this merged and continue iterating from there. Not sure if we want this in for Beta 3 (just flagging it because of the string freeze and the string changes involved).

It would be good to refactor this modal using DataViewsPicker at some point.

If this pull request is a feature enhancement, and there's a possibility that it will be refactored in the future, it might be better not to include this pull request in 6.9.

Nice work!

I'd suggest merging this as-is (we’re still in time for beta 3 if we want it included), and then refactoring to DataViewsPicker in a future iteration.

I'd suggest merging this as-is (we’re still in time for beta 3 if we want it included), and then refactoring to DataViewsPicker in a future iteration.

Understood, let's ship this in 6.9. Package synchronization for Beta3 seems to have already started, but this PR should be backported to RC1.

This PR updates strings, but I believe string updates should be allowed until the RC1 release.

cc @priethor @ellatrix

I just cherry-picked this PR to the wp/6.9 branch to get it included in the next release: e9d0e14