-
Notifications
You must be signed in to change notification settings - Fork 4.6k
DataViews: Try using 24px padding for consistency across different uses #73334
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
Changes from all commits
bf7a011
9103d01
2e9cbc3
f426b1f
79d1844
9bb205b
2b5b26a
973c826
e725c2a
2f4fcfb
8bd5892
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -265,23 +265,33 @@ export const WithModal = ( { | |
| </p> | ||
| ) } | ||
| { isModalOpen && ( | ||
| <Modal | ||
| title="Select Items" | ||
| onRequestClose={ () => setIsModalOpen( false ) } | ||
| isFullScreen={ false } | ||
| size="fill" | ||
| > | ||
| <DataViewsPickerContent | ||
| perPageSizes={ perPageSizes } | ||
| isMultiselectable={ isMultiselectable } | ||
| isGrouped={ isGrouped } | ||
| infiniteScrollEnabled={ infiniteScrollEnabled } | ||
| actions={ modalActions } | ||
| selection={ selectedItems.map( ( item ) => | ||
| String( item.id ) | ||
| ) } | ||
| /> | ||
| </Modal> | ||
| <> | ||
| <style>{ ` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may want a hasPadding prop in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed. Happy to take a look at that in a follow-up 👍 |
||
| .components-modal__content { | ||
| padding: 0; | ||
| } | ||
| .components-modal__frame.is-full-screen .components-modal__content { | ||
| margin-bottom: 0; | ||
| } | ||
| ` }</style> | ||
| <Modal | ||
| title="Select Items" | ||
| onRequestClose={ () => setIsModalOpen( false ) } | ||
| isFullScreen={ false } | ||
| size="fill" | ||
| > | ||
| <DataViewsPickerContent | ||
| perPageSizes={ perPageSizes } | ||
| isMultiselectable={ isMultiselectable } | ||
| isGrouped={ isGrouped } | ||
| infiniteScrollEnabled={ infiniteScrollEnabled } | ||
| actions={ modalActions } | ||
| selection={ selectedItems.map( ( item ) => | ||
| String( item.id ) | ||
| ) } | ||
| /> | ||
| </Modal> | ||
| </> | ||
| ) } | ||
| </> | ||
| ); | ||
|
|
||
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.
Do you still see us offering a way to configure padding later on? Or would this PR be enough for the use cases discussed?
Ideally, these styles for the card would be removed from this package, for example. If we were thinking about follow-up with a PR to configure padding via CSS Custom Prop, how would you feel about introducing it in this PR?
I'm all for controlling scope in PRs. In this situation, I worry about this: this PR ships a change that requires consumers to adapt. Then, later on, we may be shipping an API for setting padding, and they'll need to adapt again. I wonder if this experience can be more streamlined.
Uh oh!
There was an error while loading. Please reload this page.
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.
For the main use cases I've had in mind so far (Card and Modals) I think this PR might be enough to begin with, without requiring configurable padding on DataViews, with the caveat that we still need a couple of these rules. In the other thread (#73334 (comment)), I think a follow-up would be to allow Modal to zero out its padding, similar to this Card rule that still exists.
Pretty hesitant TBH. Each of these PRs has generated a lot of valid discussion but also quite differing opinions amongst reviewers and in terms of how customisation should work. My main goal is to try to advance a couple of current use cases (using DataViews / DataViewPicker in a modal, simplifying the CSS where we can).
While configurable padding is a good idea, as @jameskoster pointed out in #72989 (comment) "the juice simply isn't worth the squeeze", which is to say, we quickly encounter a lot of complexity.
So for now, my hope is to see if there's a minimal step forward for all this that we can commit to.
All that said, did you have an idea in mind for how we might structure a CSS custom variable or prop? I suppose if it were to be mostly an internal thing, it wouldn't preclude adding in a
paddingprop further down the track 🤔Edit: I replied to this comment before reading the further comments below about other DataViews cases like
16pxpadding and that Cards come in various different sizes. I think I'm a little on the fence about how all this should work 🤔