-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Administration: Add CSS custom properties for admin color scheme #10825
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| --wp-admin-color-menu-bubble-background: #{variables.$menu-bubble-background}; | ||
| --wp-admin-color-menu-collapse-text: #{variables.$menu-collapse-text}; | ||
| --wp-admin-color-menu-collapse-icon: #{variables.$menu-collapse-icon}; | ||
| --wp-admin-color-menu-collapse-focus-icon: #{variables.$menu-collapse-focus-icon}; |
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 think this is too much, and will prevent us from making any meaningful change to the profiles in the future, basically we'll be stuck with these variables forever. I'd rather avoid introducing new CSS variables if possible, or introduce the minimum possible (only the one we need right now)
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.
Why do you think we will be stuck with these variables forever? In WP, just about everything is forever, and all those variables in the respective admin scheme scss are going to prevail forever.
So why not expose them all at once and have the capacity of using them for our convenience in GB? How are they going to hinder us?
Do you mean that since they will be publicly exposed, extenders could start using them, locking us in? Isn't the plan to simply support the current color schemes?
Anyway, I would leave at least the basic ones.
--wp-admin-color-base: #{variables.$base-color};
--wp-admin-color-text: #{variables.$text-color};
--wp-admin-color-icon: #{variables.$icon-color};
--wp-admin-color-highlight: #{variables.$highlight-color};
With this exposed, we could even consider refactoring the mixins base styles, since there won't really be any more need to duplicate the colors.
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.
Why do you think we will be stuck with these variables forever? In WP, just about everything is forever, and all those variables in the respective admin scheme scss are going to prevail forever
Right now these are sass variables, so not public APIs, CSS variables on the other side are public APIs.
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.
Yes, this is what I mean. You are taking into account that if we open them we will be stuck to them forever. Anyway I'm wondering what is the proposal for the Theme provider instead. Because colors are predefined already, and for now we are duplicating them which is not ideal either.
cc @youknowriad
Trac ticket: https://core.trac.wordpress.org/ticket/64571
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.