-
Notifications
You must be signed in to change notification settings - Fork 4.6k
DataViews: Pass styles to the column. #72813
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
| titleField: 'title', | ||
| descriptionField: 'description', | ||
| mediaField: 'image', | ||
| layout: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for testing.
|
Flaky tests detected in e4cd6b3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/18929070381
|
|
Rather than arbitrary CSS for each column, could we do something more limited with the existing I do think the ability to set the outermost margin around whole dataview is different though, allowing parents to set arbitrary inline and block padding pixel values for the padding that goes all the way round the edge seems like a good thing. |
For the mobile case, for example, density feels like it could work, but in practice, relying on density for a smaller view creates exactly what you expect: a dense view. In important cases, dense views are not mobile-friendly. Click areas get small, and things start to feel rather tight. It leaves us with a bit of a dilemma: does I think we'll move towards a system for responsiveness and a system for density, both likely inherited through context. Context provided via props, or context provided via shared React context. This PR is early, and it's aggressive. I want to shake out our best guesses with something that works today, so we can see potential dead ends sooner. |
|
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. |
| ); | ||
| } | ||
|
|
||
| function getColumnStyles( view: ViewTableType, columnId: string ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any perf penalty of this regenerating an object on each render? Maybe not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in what it currently does, but it could if style logic becomes more complex.
youknowriad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach looks good to me, I'm not sure about the naming itself but I don't have better suggestions.
|
This seems closely related to #72336. |
|
I opened a CSS first alternative to this #72912. I'm curious what y'all think about that? |
WIP/Experiment
What?
This PR is experimental (and naive), allowing consumers to control CSS styles for
<td>and<th>.Why?
<DataView>styles are not modifiable without using CSS overrides that make use of internal component classnames. This is not future-friendly.What would you want to change them for?
As an example, as a consumer, maybe my mobile container padding is
10px. I should be able to set my DataViews' padding to match.How?
It utilizes the
view.layoutproperty, which already exists to provide styles to<th>and<th>.Considerations
Magic variables
I've introduced some
magicproperties to represent the ability to style internal columns like bulk actions.These are:
__default__Applies as base to all columns.__primary__Applies to the Primary column.__actions__Applies to the Actions column.I don't contend this is the best approach; however, the current code loops through the
columnlooking forstyle.[column_id]. This makes it work with minimal changes and decent backward compatibility.Theming
The default layout could be retrieved from a global theming context.
Problems
This works for the table. How do we get consistent padding for
DefaultUI... 🤔Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast