DataViews / DataViewsPicker: Add an optional padding prop with named values #72989
Conversation
andrewserong
added
[Type] Enhancement
|
I'm not 100% sure of the approach here, so I'll leave this as a draft for now, but very happy for any feedback. |
|
Size Change: +1.13 kB (+0.04%) Total Size: 2.54 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 860c35e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/19560136268
|
|
I've taken a similar approach in #72912, the main difference being that you create the CSS variable in the DataView, I create it in the container (the card in my example). The ability to have more granular control on padding in your approach is more flexible and I don't think there is a best here. With that being said, I lean towards the container providing that context, DataViews inheriting it, or using its own default when context is not provided, as a more flexible starting point. Once we introduce padding controls for DataViews, it will be hard to take them back. However, if we start by moving padding into overridable variables, we don't have the pressure to commit to an API right now. Yes, third parties can override the CSS variables, but that's a much better problem to have (and arguably not a problem). It also moves us toward something like the Open/Closed Principle, where layouts mostly evolve through composition and overrides rather than by changing component APIs. |
Glad we honed in on similar approaches! If we can handle the case for the |
|
I just noticed that Card Component Padding System Enhancement merged, giving comsumers the ability to pass in spacing tokens into the component. The approach you take here would be more aligned with that. |
Oh, thanks for flagging that. I like that it uses a |
| selection={ selectedItems.map( ( item ) => | ||
| String( item.id ) | ||
| ) } | ||
| paddingX={ 32 } |
There was a problem hiding this comment.
I recognise it won't be possible currently, but it would be excellent if this value could reference a semantic spacing token to ensure it stays in sync with any future modal padding changes. cc @aduth.
There was a problem hiding this comment.
Some possible suggestions for making this future compatible could be to (1) use a syntax similar to #72511 and #72984 with object containing logical properties like "block" and "inline", and (2) maybe have this behave as a multiplier of the 4px base spacing (i.e. 8 unit multiplier = 32px), which is how I've been thinking of numeric values in #72984 and #73308.
There was a problem hiding this comment.
Oh, I like the idea of using a controlled set of tokens, that's looking promising. If we went with that approach, what might we call the prop. Would we call it size for consistency with Card, or simple call it padding (with no x).
I suppose one challenge with going with configurability here is that we also need modals to be able to zero out their padding. Part of the idea in #73334 was that we could make that an implicit part of using DataViewsPicker within a modal, whereas this PR will require consumers to configure things a little more manually. That mightn't be terrible, though.
Happy to continue iterating here, let me know if anyone feels strongly about one approach or the other!
There was a problem hiding this comment.
I don't know that I find size to be a particularly fitting name, for this use-case or for card. At the same time "padding" is a very low-level concern that ideally developers don't need to manipulate. In the future I'd like if DataViews might use something like Box and density as being explored in #73215 to influence padding without this kind of component-level configuration. But that probably wouldn't ever support a complete padding reset.
With all that in mind, as far as suggestions:
- I think a prop like
padding={ { inline: 0 } }could be fine - If the goal is to supporting "zero-ing out" padding, we could consider expressing this a different way like a
seamlessorpadded=falsebinary option between default padding and zero padding
There was a problem hiding this comment.
Oh, interesting, thanks for the continued ideas! I've started playing around with this and have pushed an update — it's not quite ready for review, I'm just saving my progress from hacking around with Claude on some of these ideas. I'll see if I can tidy things up tomorrow.
There was a problem hiding this comment.
In keeping with the named values theme, I've gone with none for zeroing things out for now.
| .dataviews-picker-modal .components-modal__content { | ||
| padding: 0; | ||
| } |
There was a problem hiding this comment.
Should we build this into the componentry rather than just the story?
There was a problem hiding this comment.
How might we build it in? Were you thinking of configurable padding on the Modal component as well, or adding this as a hard-code rule somewhere?
There was a problem hiding this comment.
I guess I was hoping to avoid this cross-component style override somehow. It relates to your point about zeroing modal padding. It remains unclear if that's something we want to commit to right now; it's quite a big change to address this relatively small issue.
Perhaps an alternative approach would be to set paddingX to 0 when DataViews or DataViewsPicker is inside a modal (just in the story, not using any logic). Then we can add some zero-padding-specific styles to DataViews (maybe something like this). We could use the exact same technique in the Card story, if we want to.
This would enable consumers to use DataViews either way in their own environments; padded with full-width strokes, or unpadded.
There was a problem hiding this comment.
Mmm, interesting ideas! I like the idea of being able to support both. I haven't quite gotten to playing with this yet. I've pushed an update to this PR, but it isn't quite ready for review yet... I still need to do some more hacking on it 😄
There was a problem hiding this comment.
(just in the story, not using any logic)
I've updated the Story so that I've removed the padding rule for Modal and set the DataViewsPicker to use the none value to demo what it looks like with the padding zeroed out on the DataViewsPicker.
In practical usage, consumers could decide how they want to use this — i.e. if they want to use zero padding on the DataViewsPicker or if they want to zero out the modal itself... so I think I'm liking this padding prop, as it gives quite a bit of flexibility.
andrewserong
force-pushed
the
try/dataviews-with-custom-padding-x
branch
from
8c13d21 to
e0c965b
Compare
andrewserong
force-pushed
the
try/dataviews-with-custom-padding-x
branch
from
75bf716 to
61dfe54
Compare
andrewserong
changed the title
| /** | ||
| * Maps padding size tokens to CSS values. | ||
| * Aligned with the Card component's spacing scale (space function with 4px base): | ||
| * - x-small: space(2) = 8px | ||
| * - small: space(4) = 16px | ||
| * - medium: space(6) = 24px | ||
| * - large: space(8) = 32px | ||
| * - x-large: space(12) = 48px | ||
| */ | ||
| const paddingValuesBySize: Record< PaddingSize, string > = { | ||
| 'x-small': '8px', | ||
| small: '16px', | ||
| medium: '24px', | ||
| large: '32px', | ||
| 'x-large': '48px', | ||
| none: '0', | ||
| }; |
There was a problem hiding this comment.
Very happy for feedback on the naming and sizing of this, maybe x-large should be 40px and there should be an xx-large with 48px? My hunch is that we'll want 48px to be in the scale as that's the current padding.
|
Alrighty, I've done quite a few updates on this and I believe it's ready for review and testing. I've been staring at this code for a little too long, so it's quite possible I've missed things, but my goals have been to:
I've updated the Default DataViews story and the WithModal DataViewsPicker story so that they can be tested with the padding prop, and you can select values to try it out. Happy for any and continued feedback of course! I'll be working on other things next week so I may be slow to implement further ideas, but I'll do my best! |
|
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. |
andrewserong
requested review from
StevenDufresne,
aaronrobertshaw,
ramonjd,
talldan and
tellthemachines
|
Hey, thanks for all the discussions, prototypes, and conversation to figure out this one. I wanted to surface a comment I've made in another place. If we introduce CSS Custom Properties as the way to style DataViews, etc., this needs to be treated as public API:
I'm happy to help figure out how to better test this, but I'd appreciate it if you all bootstrap a new README section in DataViews for this. |
| .dataviews-footer { | ||
| padding: $grid-unit-15 $grid-unit-30; | ||
| padding-block-start: $grid-unit-15; | ||
| padding-block-end: var(--wp-dataviews-padding-block-end, #{$grid-unit-15}); |
There was a problem hiding this comment.
I asked the same question in the other place, resurfacing here as well:
I've noticed that we're using
--wp-*in existing components, and--wpds-in other places. When do we use one, and when do we use another? I haven't found anything that clarifies that, and would like to understand which one should we use here.
There was a problem hiding this comment.
--wpds- tokens come from from the Theme package and are consumed by the components in the new UI package.
Eventually we'll want this variable to reference one of those tokens for the fallback instead of grid-unit-15. E.g.:
var(--wp-dataviews-padding-block-end, --wpds-dimension-padding-surface-lg)
I don't have any strong feelings about how the DataView-specific variables should be named.
| isItemClickable={ () => hasClickableItems } | ||
| defaultLayouts={ defaultLayouts } | ||
| config={ { perPageSizes } } | ||
| padding={ padding } |
There was a problem hiding this comment.
Why do we need this property? Perhaps I've misinterpreted the conversation in this PR and this other one, so correct me if I'm wrong: aren't we promoting the use of CSS Custom Properties to style DataViews? If so, why having both things?
cc @jameskoster
There was a problem hiding this comment.
In other words, how consumers are expected to tweak the padding (or background in the other PR):
:root {
--wp-dataviews-padding-block-start: /* ... */
}or
<DataViews
padding={ ... }
/> I thought Jay argued for the 1st approach, given that's how other components in the upcoming new design system (@wordpress/theme and @wordpress/ui) and the existing @wordpress/components work.
There was a problem hiding this comment.
I don't know if this is a good reason, but we might want to tweak some styles when the padding is a specific value (0), and a dedicated property can enable that(?).
| control: 'boolean', | ||
| description: 'Are the items clickable', | ||
| }, | ||
| padding: { |
There was a problem hiding this comment.
One thing on my mind is how do we test this public API? Starting by allowing folks to change them in the storybook sounds a great first step.
I'd like to give this public API more visibility and easier ways of testing and detect regressions. We could introduce a new story (name TBD, perhaps "Styles"? "Design Tokens"?, any works for me) where we expose every one of the design tokens we offer in DataViews. In this case, we should add the following controls:
- padding block end
- padding block start
- padding inline end
- padding inline start
| .dataviews-loading, | ||
| .dataviews-no-results { | ||
| padding-inline: $grid-unit-30; | ||
| padding-inline-start: $grid-unit-30; |
There was a problem hiding this comment.
My understanding was that the benefit of introducing an API for styling DataViews was that we'd no longer need container specific styles. Why do we still require these styles for the card? Shouldn't they be removed?
|
As you pointed out before, with 0 padding certain elements are getting cut off, e.g. the focus ring on the view options button: 
I was a bit surprised to see a different row-hover style in DataViewsPicker table layout compared to regular DataViews. Is that intentional, or maybe a separate issue? Let me know and I'll open one to track. I think the padding prop should only affect horizontal padding; vertical spacing seems better left to density? This highlights another potential issue with this approach... We only really need padding customisation for table and list layouts—grid layout basically never needs any padding. So I began to wonder whether padding should be customisable per layout, and I'm struck once again with how much complexity we're adding here basically just to preserve the edge-to-edge border design in table and list layouts 😅 Imho the juice simply isn't worth the squeeze, and adding a bunch of complexity to account for an inconsequential design choice just feels backwards. Obviously it goes way beyond the scope of this PR, but out of curiosity I wanted to see how critical this detail is as we think about the broader admin design. This is a super-quick mockup, but as far as I'm concerned it demonstrates a nice design is perfectly achievable without being dependent edge-to-edge borders. So I have to ask, if we didn't treat this one detail as a constraint, would we still want to add all of this complexity? 
|
|
Thanks for all the discussion, folks! Before continuing to polish this PR, I would like to get a sense of whether or not we think it's worth pursuing further as it does add a fair bit of complexity.
I agree. I quite liked an earlier PR we were looking at that settled on standardising on What do folks think? Is it worth pursuing customisability, or should we settle on DataViews remaining opinionated about its padding, but potentially making it a little simpler? |
|
Now that #73334 has landed (standardize on |
What?
Part of #72336
Allow more flexibility when it comes to padding for the DataViews and DataViewsPicker components by adding a
paddingprop with a controlled set of values.This will help consumers improve the appearance of the DataViewsPicker when used in a
Modalcomponent, by allowing folks to customise the padding. This PR updates the WithModal story to demonstrate how it can be used.Why?
As discussed in #72336, we need the padding of the DataViewsPicker to play nicely with modals, but it's also a little tricky to implement while maintaining backwards compatibility. That issue discussed a lot of different options, and this one balances backwards compatibility (if no
paddingprop is passed in the styles are the same as on trunk) while allowing the component to be customised for different use cases.It might not be the best API for the purposes, but it's one possible avenue.
How?
paddingprop toDataViewsandDataViewsPickerthat sets CSS variables for vertical and horizontal (outer) padding on DataViews/DataViewsPickerDataViewsPickerwithin the modal to zero.So far, this PR allows the following values for padding (this follows what was recently implemented for the
Cardcomponent in #72511 and was suggested by @aduth in #72989 (comment)):These can be set as string values, or within an object with properties for
block,inlineand so on, to define padding for a particular side or axis.Very happy for feedback and ideas on the naming and values, especially for consistency across components!
Testing Instructions
npm run storybook:dev)paddingcontrol in the Story to adjust the level of padding and see how it looks and feelsScreenshots or screencast