Size Change: +69 B (0%)

Total Size: 2.54 MB

_{compressed-size-action}

This approach works for me

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: jameskoster <[email protected]>
Co-authored-by: oandregal <[email protected]>
Co-authored-by: andrewserong <[email protected]>
Co-authored-by: ntsekouras <[email protected]>
Co-authored-by: youknowriad <[email protected]>
Co-authored-by: t-hamano <[email protected]>

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

@oandregal I just wanted to point out that this feels closely related to #72989. There seems to be a growing need for more flexiblity about how certain DataViews aspects are styled in different contexts. I don't know if that changes your perspective at all?

@jameskoster I've been catching up on @wordpress/theme and @wordpress/ui work. I still think it's super early days for all of this, and worry that we may change direction. You've been more involved there, so I'll trust your judgment here.

For me, this is good to go as long we treat this as public API:

• It needs a "Styles/Design tokens" section in the README.
• It needs tests.
• We document breaking changes in the CHANGELOG.

All of that shouldn't block this PR. 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 (doesn't need to be in this PR, but it needs to happen).

@jameskoster I've got a follow-up question about the approach at https://github.com/WordPress/gutenberg/pull/72989/files#r2549149227 I thought you argued for using CSS Custom Properties for styling DataViews, or did I misunderstand?

I still think it's super early days for all of this, and worry that we may change direction.

We may change direction, but would you agree that DataViews should 'work' on different background colors regardless to increase portability? Or do you think this is too much of a stretch technically?

To elaborate a bit: in @wordpress/theme we provide a small set of semantic color tokens intended for container backgrounds (e.g. components like Page, Card, Modal, etc.). They range from light gray to white in the neutral color scale. At the moment, DataViews only looks correct when rendered on a white background specifically.

So based on the assumption that we want DataViews to look correct on different backgrounds we need a strategy to ensure compatibility with those tokens. That way, if we decided (for example) that Modals should have a light gray background then DataViews rendered in a Modal would 'just work'.

I acknowledge an alternative approach would be to say DataViews always has a white background. But if we go in that direction we may need to adapt the design a bit to account for the fact it can render in many different contexts like the modal example above.

This all ties in very closely with github.com//issues/72336.

We may change direction, but would you agree that DataViews should 'work' on different background colors regardless to increase portability? Or do you think this is too much of a stretch technically?

Yeah, I agree.

We've passed the "prototype phase" and we are now in the "expand phase" for the library. We need to tie it to the larger efforts (design system, etc.), which means support theming concerns. Using CSS Custom Properties a public API for styling/customization makes sense, and I've used that in the past in a smaller scale project. There are implementation details that are not clear to me yet (how do we handle breaking changes, how do we test, what are the proper tokens to expose, how do we tie this with WordPress admin themes, etc.). Those are important concerns to figure it out, but don't need to all clear before merging this PR.

I think the following approach could also be considered. Note that I removed the wp prefix:

.dataviews-wrapper,
.dataviews-picker-wrapper {
	--dataviews-color-background: $white;
	background-color: var(--dataviews-color-background);
}

This is similar to the approach taken to allow the size of checkboxes to be overridden, for example.

The advantage of this approach is that it limits the scope of the CSS custom property to the DataViews package. Consumers are forced to explicitly define a CSS custom property at the same hierarchy if they want to override the color:

.my-app {
	.dataviews-wrapper,
	.dataviews-picker-wrapper {
		--dataviews-color-background: #ddd;
	}
}

I've just given this a rebase to fix merge conflicts. Any objections if we merge this PR in its current state? It looks like the main question is whether we're happy with the --wp-dataviews-color-background naming convention or if we should go with something else.

I don't have a strong opinion re: the naming, but it would be good to land this PR in time for Gutenberg 22.2 so that the bug introduced in #73240 doesn't make it into the release.

I've pushed documentation. I am still unsure how can we add tests for this, in the meantime, I've added it to storybook.

The advantage of this approach is that it limits the scope of the CSS custom property to the DataViews package. Consumers are forced to explicitly define a CSS custom property at the same hierarchy if they want to override the color:

This means consumers need to know the internal structure of the DataViews component, and if it changes it'll be a breaking change. If we were to set an initial value for the property, I'd rather do it with a :root{ ... } or something along those lines, so consumers don't need to do anything special to override the value.

Thanks for landing this, @oandregal! 🙇