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: andrewserong <[email protected]>
Co-authored-by: youknowriad <[email protected]>
Co-authored-by: StevenDufresne <[email protected]>
Co-authored-by: ramonjd <[email protected]>
Co-authored-by: oandregal <[email protected]>
Co-authored-by: ntsekouras <[email protected]>
Co-authored-by: jameskoster <[email protected]>
Co-authored-by: jasmussen <[email protected]>
Co-authored-by: aduth <[email protected]>
Co-authored-by: fcoveram <[email protected]>
Co-authored-by: p-jackson <[email protected]>
Co-authored-by: tellthemachines <[email protected]>
Co-authored-by: talldan <[email protected]>

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

Size Change: -1.24 kB (-0.05%)

Total Size: 2.58 MB

_{compressed-size-action}

This is how it's looking in the site editor, where it's now further to the left / right than the header. If we go with this, should we also update the padding on the .admin-ui-page__header?

IMO it seems like it, or adjust some other css in site editor to add the extra padding to look like in trunk.

This is additional CSS, but it's hopefully not too intrusive

I guess this is the drawback with this approach, we're still forced to add custom css to make DataViews behave as we want in certain circumstances. I agree it's not that intrusive, but it doesn't seem very scalable. We can't add these special rules for all combinations.

I guess this is the drawback with this approach, we're still forced to add custom css to make DataViews behave as we want in certain circumstances. I agree it's not that intrusive, but it doesn't seem very scalable. We can't add these special rules for all combinations.

That's a good point. But how many combinations do we think we'll need to handle, and is it worth it to attempt to codify that just yet?

I guess what I'm curious about is: how do we make a pragmatic decision in the shorter-term to a) make DataViews look acceptable in modals, while b) simplifying the CSS without creating too much of a backwards compatibility headache.

I don't think this PR is perfect by any means, but is it a reasonable step in the direction we'd like to go in? Or to put it differently, is it proposing a change that's going to make things harder for us further down the track?

Happy to try out any other ideas, too. My gut feeling is that we should probably go with a pragmatic fix in the shorter-term, and accept that we might need to refactor these rules again further down the track when we've got a clearer idea of how we'd like to handle theming for these kinds of components 🤔

My gut feeling is that we should probably go with a pragmatic fix in the shorter-term, and accept that we might need to refactor these rules again further down the track when we've got a clearer idea of how we'd like to handle theming for these kinds of component

I'd also like to know the stakes are.

That is, what do folks need right now vs what can be adjusted later when we have more information about the cases we need to cover?

As far as I can see, the scalability concern is valid, but context-specific overrides already exist (Card, Modal), and this simplifies the base padding while improving Modal appearance.

Modals are one example, Cards are another. Obviously we can't control how third party consumers compose things. Someone might want to place a DataViews instance in their own custom container component... In such a scenario it would ideally 'just work'.

That makes me biased toward finding the solution that will make dataviews as portable as possible, with as little consumer effort as possible. I remain drawn to the 'no padding' approach, mostly because it's simplest. But if we're not satisfied with that then I think making the padding configurable rather than adding a bunch of styles to handle specific use cases would be preferable. I suppose that's the approach you were working on in #72989 :)

Thanks for the feedback on this one, folks! I'll close this out in favour of #72989 which seeks to make padding customisable while maintaining backwards compatibility for components not setting a padding value.

I'm just re-opening this PR to flag that it's still under consideration as one of the (many) options for addressing padding.

One more thing :) I realised this issue is basically the exact same problem we faced when BaseControl supplied it’s own opinionated margin: #38730. The solution was to remove the margin altogether, deferring spacing logic (beyond the internal layout) to the container.

For instance the gap between the modal title and the search/filter UI here is enormous and looks very clunky imo:

There should be no gap added by the DataViews component itself, if there's it should be removed in its own PR (making DataViews more portable)

@jameskoster I see you care a lot about removing the padding entirely from DataViews. I personally still think this is a design decision but my hunch is that you may be misjudging the potential impact (on existing users of DataViews). Regardless, the current PR doesn't change the statusquo, it just makes things mode clear, remove useless container queries and makes things more consistent. So we should ship it and it wouldn't impact the decision about whether we remove all the padding entirely later from DataViews.

So I propose that we merge this as is. And we continue exploring padding-less dataviews, which if it works design wise has my preference because there's no potential flexibility that creates misusage.

Yeah I was thinking I might resurrect the zero-padding PR to explore further. But I agree it doesn't need to prohibit merging this one first.

Before we do I would appreciate a final confidence boost from @WordPress/gutenberg-design about reducing Modal padding from 32px to 24px, and obviously fixing the alignment issue noted in this comment.

Thanks folks! I've rebased again and pushed a small fix for the patterns modal. Its sidebar uses absolute positioning so needed to have its padding updated to match the new modal padding (done in 8bd5892).

Here's how it's looking now:

I notice that the padding of the header of the modal does look a little large. It's a similar problem to this topic:

For instance the gap between the modal title and the search/filter UI here is enormous and looks very clunky imo:

For the case of the pattern explorer at least, things get interesting when you scroll down the modal as this PR looks neater than trunk in that situation. It appears that trunk has a smaller padding-bottom on the header to compensate, but it can look a bit awkward. Here's how things look when scrolling down a modal (this PR wins out):

This PR

Trunk

On this point, if we plan to allow customisation of DataViews padding then couldn't a consumer set DataViews padding to 0 thereby surfacing both of those problems anyway? Isn't it something we'll need to fix regardless?

Yes. For me this problem is similar to the rest of the discussion surrounding customisability, which is to say, these discussions are important and touch on so many points that are worth digging into and resolving. My preference would be to do all this work in small(er) iterative stages. I'm imagining that first, we merge this PR to standardize on 24px and then subsequent PRs can look into adding customisable padding and start addressing the other problems we've identified.

Overall, I'm really pleased to see everyone so engaged in this topic! I'm aware that this is still a breaking visual change (albeit a relatively small one), so it's hard to reach consensus here:

Before we do I would appreciate a final confidence boost from @WordPress/gutenberg-design about reducing Modal padding from 32px to 24px, and obviously fixing the alignment issue noted #73334 (comment).

Yes, I'd love to feel confident in landing this one. Please do raise any concerns if anyone thinks we shouldn't land this PR. Otherwise, I'll aim to merge this in, in around 24 hours.

Thanks again, all!

I'm going to push the button here :) you know, nothing is frozen :P

It seems this PR introduced unintended padding into the global styles sidebar, which is probably where this is relevant.

This PR appears to have introduced unintended padding into the global styles sidebar.

These sections are likely related to the issue:

• https://github.com/WordPress/gutenberg/pull/73334/files?diff=unified#diff-8a3b5b97c7883702b694956c858077b83459c136f61cc7f5baf36ff438626627L13-R14
• https://github.com/WordPress/gutenberg/pull/73334/files?diff=unified#diff-da91bf2d63ee6da3db680ce9b53ab5cf098a7b117b20e5665b071d8e36ca0bdfL27-R40

Here are some examples of the issue:

Edit Palette

Before	After

Font size presets

Before	After

Blocks

Before	After

Oh, good catch @t-hamano! I can put up a PR later on today for that.

@t-hamano I've put up a revert for the changes to the Global Styles UI areas over in #73834. It looks like those changes also rolled out 24px padding changes to areas that aren't strictly speaking in scope for the intended DataViews and Modal changes. I think it'd be fine to revert those parts.

FYI @youknowriad in case you wanted to keep them (we can always look at bringing those changes back after the revert, of course)

The Global Styles changes were intentional because I wanted a coherent padding for global styles both in the styles panel and in the styles page. But I'll check in your linked PR.