DataViews: Inherit styles from parent card. #72912
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +377 B (+0.02%) Total Size: 2.38 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 445a956. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/19021012858
|
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
1 similar comment
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
Contributor
|
Just catching up on the idea here. So the idea is that other instances, e.g. It's a compelling idea, particularly because it'd require consumers to do less manual configuration than what I'd explored in #72989. Still, I like that what we've both gone with is using a CSS variable and a fallback. The question is whether or not the CSS variables are defined by a parent, or explicitly via a DataViews / DataViewsPicker prop? The fact that this PR handles things implicitly is quite nice... One other detail is that I suppose for the other cases like gutenberg/packages/dataviews/src/components/dataviews/style.scss Lines 112 to 114 in e0211c4 Keen to hear what other folks think about these ideas, too. |
Contributor
Author
Yes.
It's a great callout and a challenge with this approach. I feel like a container could wrap the component and pass a zero padding state. So you could get something like <Modal size="large"> // Modal Spacing get applied to itself
<Box space={0}> // Passes no padding to the DataView
<DataView />
</Box>
</Modal>or something of that nature.... hmm maybe that's not better than adding props to |
p-jackson
reviewed
Nov 13, 2025
| }, [ isBorderless, size ] ); | ||
|
|
||
| return ( | ||
| <ContextSystemProvider value={ contextProviderValue }> |
Member
There was a problem hiding this comment.
The size is still being passed into the context system here. Should it be removed? Maybe it should be kept for some future purpose where JS needs to know what the size is set to 🤔
| <View { ...otherProps } ref={ forwardedRef }> | ||
| <View | ||
| { ...otherProps } | ||
| data-wp-container-size={ size } |
Member
There was a problem hiding this comment.
I think using a data attribute is going to be over specific. From the specificity calculator
As a user of the component library you're going to want to define your own .dashboard-dataview-mobile class to override the CSS variable, but you're going to have to do something janky in order to make it specific enough.
Rather than putting all the styles on the component with cx() and turning them on and off with a data attribute, what if you only one padding style to cx() based on the current value of the size prop. That way the CSS variables will be defined using a selector with a single classname. Users of the component library can then define their own single classname to override the variable.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related: #72336
What?
This is an alternative #72813 which passes all CSS properties to the column to allow DataView customizations. The approach taken in #72813 is similar to #72511 which also passes CSS directly to the element.
This PR takes a different approach to solving hard-coded spacing values in the DataView by using the Card to declare its paddings using CSS so they can be used by descendants.
Why?
Descendant components don't have access to their container's spacing context but they arguably should and we have hardcoded relationships between specific components like DataViews because it's missing. Example.
How?
Set
[data-wp-container-size='{size}']on the Card element that maps to a CSS style that sets the following CSS variables to be consumed by descendants:--wp-container-inline-padding--wp-container-block-padding--wp-container-inline-start-padding--wp-container-inline-end-paddingWhy not
--wp-container-block-(start|end)-padding? No specific reason. It isn't necessary yet.Why
-container-This PR implements the style in the Card component, but this approach could be applied to any container. The browser will search up the tree to resolve
--wp-container-block-padding. This could also work if you had a different type of container like a<Box>,<Column>or<Modal>, and would work with nested components.Testing Instructions
WithCardstory indataviews.story.tsx'xSmall' | 'small' | 'medium' | 'large'Testing Instructions for Keyboard