Repository navigation
Fix dataviews scroll and footer stickiness #72150
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,11 @@ | ||
| .admin-ui-page { | ||
| display: flex; | ||
| height: 100%; | ||
| background-color: $white; | ||
| color: $gray-800; | ||
| position: relative; | ||
| z-index: 1; | ||
| flex-flow: column; | ||
| container: admin-ui-page / inline-size; | ||
|
|
||
| @media not (prefers-reduced-motion) { | ||
|
|
@@ -41,3 +45,25 @@ | |
| } | ||
| } | ||
| } | ||
|
|
||
| // Show button text labels when preference is enabled. | ||
|
Contributor
There was a problem hiding this comment. This is very adhoc to me, and has nothing to do with the Page component, What if folks use other UI components in the header. This is introducing an implicit dependency that is very easy to regress.
Contributor
There was a problem hiding this comment. I think ideally this should be a prop in the Button component itself (like "autoSwithToText" or something) and
Member
There was a problem hiding this comment. Is this meant to apply universally across the application? If no, then it seems appropriate as an application-specific component that wires the preference to the specific button rendering, making it clear in the component naming to be choosing this behavior. If yes, then my first instinct would be to question the choice of a differing experience at a foundational level and still preferring an application-specific component, but perhaps it could be configurable at a "theme" level. I don't know that it would need a specific button prop in that case though.
Contributor
There was a problem hiding this comment.
It is meant to apply in a lot of places (block toolbars), header icons... I think if you ask folks who activate it, they want it for all icon buttons. That said, it's often hard to guarantee that they'll all work well automatically and this is probably why we're using more of a "curated" approach. For me, this is a fundamental piece of the
Contributor
Author
There was a problem hiding this comment. Button text labels work via a classname applied to the body when the preference is enabled, and local styles in various parts of the codebase. If you search gutenberg for I agree the implementation could be better but am led to believe by the discussion in #61763 that adding it at the component level is not so straightforward.
Yes! I use it because I can't tell most of the icons apart from each other 😅 so it would be very helpful to have everywhere. They do often break because folks don't remember to test the preference or sometimes even to design with it in mind (possibly not everyone knows it exists).
Member
There was a problem hiding this comment. Aha, thanks for the reference to the issue. I should have expected it'd already been discussed at length 😅 I had seen how the class is applied in a bunch of different places which is what led me to suspect it's not uniformly applied. With some of the challenges outlined in #71196 (specifically Button's bloated API), revisiting those abstractions might help with the challenges raised in #61763 around baking this into the Button component. For example, if we had a dedicated component for "icon buttons", it may be easier and more appropriate for this behavior to be built-in. |
||
| .show-icon-labels { | ||
| .admin-ui-page__header-actions { | ||
| .components-button.has-icon { | ||
| width: auto; | ||
| padding: 0 $grid-unit-10; | ||
|
|
||
| // Hide the button icons when labels are set to display... | ||
| svg { | ||
| display: none; | ||
| } | ||
| // ... and display labels. | ||
| &::after { | ||
| content: attr(aria-label); | ||
| font-size: $helptext-font-size; | ||
| } | ||
| } | ||
|
|
||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,5 @@ | ||
| .edit-site-post-edit { | ||
| padding: $grid-unit-30; | ||
| } | ||
|
|
||
| .dataforms-layouts-panel__field-dropdown { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -254,7 +254,6 @@ export default function PostList( { postType } ) { | |
| ) } | ||
| </> | ||
| } | ||
| > | ||
| <DataViews | ||
| key={ activeView } | ||
|
|
||
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.
Can we keep this prop please. I know it's not used today but most screens that are not dataviews will need it (think settings, style book).
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.
In other words, I don't want this to be left to the consumer.
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.
If #72135 is an accurate preview of those new screens it doesn't seem like they'll want padding, because it would interfere with the bottom border on the tabs going full width.
I'd like to better understand what's the concern here. Is it that third parties won't use the correct padding?
If the goal is to ensure that third party screens match the padding used elsewhere in the UI, then we'd probably be better off making some CSS custom properties available to consumers of the components, like
--container-vertical-padding,--container-horizontal-padding. That way any screen can have tabs or whatever and add the padding only where it's needed.Another possibility if that tabbed interface is going to be a common one: we make the tabs component available for third parties to build their screens and add the padding to the content areas ourselves.
Otherwise, if we really want to keep this behaviour in the Page component, it's better to make the hasPadding prop false by default, because it doesn't seem to be the majority use case, and render the extra wrapper only if hasPadding is true.
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.
The padding is not around the tabs, it's about the left and right padding (and bottom) of the content area.
My concern is that it's not the role of the consumer to define the padding of the surface, it should be consistent for all pages.
I disagree, our main reusable bits are components, I'd like us to avoid CSS custom properties between different random CSS files as much as possible. These are not strongly tied together and bugs can arise very quickly. There's very little we can do to statically analyze usage...
I think this is just incidental of the fact that we started with dataviews page and that these components are meant to span full width. But IMO, there are way more use-case first and third-parties where padding is needed.