-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Routing Boot Package: Remove margin and border-radius from layout components #75053
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
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
this only solves the issue partially for me. I think the "extensible site editor"'s sidebar color should also adapt to the profile. and I'm on the fence about removing the padding on the fonts screen, it's good that it's closer to site editor design, kind of bridges both. |
|
Yes :) |
I think we are going to need to work on a Core patch to expose these colors cleanly for Gutenberg. Let me check the thing, and I will ping you there to see if we can make it. |
|
Added upstream: #64571 |
| color: var(--wpds-color-fg-content-neutral, #1e1e1e); | ||
| isolation: isolate; | ||
| background: var(--wpds-color-bg-surface-neutral-weak, #f0f0f0); | ||
| background: var(--wp-admin-color-menu-background, #1d2327); |
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.
I've also switched to #1d2327 as this is the default base color for the Default color scheme. But I can revert back to the #f0f0f0 fallback
Don't we already have these variables already Or is this sidebar color not the "main color" |
Nope, the sidebar is using the We could extend the base color there, but I think that is a duplication that we should be avoiding. As this grows, it's going to be coupling more and more variables that could actually be brought from where they belong (Core). |
The plan at the moment is that the variables should be coming from |
I feel a bit divided with this approach. We keep duplicating color codes, instead of simply exposing them. It's true that the ThemeProvider aims to color ramp for the whole palette so we don't need to set parameters manually and all start from the seed of what they called "primary colors" And I could also understand that the long-term target is to completely override all things style/reactivity, but given all the BC historical troubles, I feel that admin colors are going to prevail somewhat forever, this is why I believe we are rewriting unnecessary code over and over again even if they are going to be taking just the "primary colors". I've seen the primary color already hardcoded in 3 places (the mixin, the theme and one story). In my head this doesn't make any sense. If we exposed this from core, we could have taken it just from one place for all 3 packages. Also I've drafted some code using the ThemeProvider, and the new color ramp algorithm of the ThemeProvider, would end setting something like Still we could draw, even a single seed color, all the colors from Core to avoid having to duplicate everything (maybe also the background as currently the difference is so massive?) EDIT: This is some idea that came to my mind on an attempt of unifying everything #75096 based on the Core PR |
|
I'd personally agree that it would be nice for WordPress core to provide the "seed" color more directly, rather than what we've had to do with I think this use-case is interesting since it's not immediately clear to me if this little border radius edge is part of the "boot" app, or if its part of the surrounding admin chrome, which could have an effect on how we'd want to implement it. From a design system perspective, my impression is that if we want the "boot" app to operate entirely within the theming tooling (which I do), we should be using theme CSS properties and seed inputs to generate those ramps. Here, I see two layers: a backdrop (i.e. surrounding UI chrome emulating an extension of the admin sidebar) and the main canvas area of the application. Each of these have a different background color, with the backdrop using the user color preference, and the canvas being white currently. I'm having trouble getting my environment running correctly to iterate on this screen specifically at the moment, but my sense is that we already have the basis for this in the Root component, but that the current hard-coding should be considering the user preference?
(may need some more consideration of how to interoperate in both the "embedded" and "full-page" implementations) |
Yes, you read my mind. Exactly something like this is what I proposed in #75096, currently only having |

What?
Continues #75036
Why?
@youknowriad commented that when adding Styles in profile, the background was popping for the new layout components
How?
Instead of removing the background, which may make sense in the future with a future admin redesign, simply fully expanding the margins to cover the whole scene and removing the rounded corners fixes the thing while maintaining all backgrounds.
Testing Instructions
Screenshots or screencast