-
Notifications
You must be signed in to change notification settings - Fork 2
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
[MOOSE-99] Moose 2.0 #152
[MOOSE-99] Moose 2.0 #152
Conversation
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.
Left some questions & suggestions, Geoff. This is great. We'll review at the biweekly together to get feedback before we approve to merge.
--color-neutral-10: var(--wp--preset--color--neutral-10); | ||
--color-neutral-20: var(--wp--preset--color--neutral-20); | ||
--color-neutral-30: var(--wp--preset--color--neutral-30); | ||
--color-neutral-40: var(--wp--preset--color--neutral-40); | ||
--color-neutral-50: var(--wp--preset--color--neutral-50); | ||
--color-neutral-60: var(--wp--preset--color--neutral-60); | ||
--color-neutral-70: var(--wp--preset--color--neutral-70); | ||
--color-neutral-80: var(--wp--preset--color--neutral-80); | ||
--color-neutral-90: var(--wp--preset--color--neutral-90); | ||
--color-neutral-10: var(--wp--preset--color--neutral-10, #f5f5f5); | ||
--color-neutral-20: #e8e8e8; | ||
--color-neutral-30: #d1d1d1; | ||
--color-neutral-40: #a3a3a3; | ||
--color-neutral-50: var(--wp--preset--color--neutral-50, #767676); | ||
--color-neutral-60: #5d5d5d; | ||
--color-neutral-70: #464646; | ||
--color-neutral-80: #2f2f2f; | ||
--color-neutral-90: #1f1f1f; |
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.
Curious about this change. IIRC, our goal was to always define all properties via theme.json and then reference those values in our CSS to prevent mismatched values.
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 issue I've been running into on projects lately is that design wants all the colors available, but they don't want all the colors available to editors. So, we end up only adding a few of the colors to the global color palette and then the rest get added in code to be used elsewhere on the site. Normally these other grays are used in places not editable by editors like buttons and navigation. It's likely though that we're not using all of these colors and a lot of them can be stripped though.
Design also wanted to go back to primary
/ secondary
with this update - so maybe we have a pretty big disconnect on colors that we need to meet to go over? Maybe I can post in the channel first for feelers on if we need to meet or not?
There's always the option to get super granular with colors too, where we can choose per block which colors are available in the palette.
--color-blue: var(--wp--preset--color--blue, #3050e5); | ||
--color-blue-rgb: 48, 80, 229; |
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.
Couldn't the blue preset value be the rgb() color?
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 wasn't able to find a combination of this that worked. It doesn't appear you can just assign 48, 80, 229
to the preset value. It expects a prefix like #
or rgb(
. Setting it to rgb(48, 80, 229)
doesn't allow you to use in an rgba()
definition.
Since this color is only used for the ::selection
code, I could also just one-off the rgba()
definition to the selection code instead?
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.
Let's keep it defined as a color var, but maybe name it specifically like --color-selection-highlight
or something similar?
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.
Updated to new naming for variable; Moved it to it's own section in the colors file; Added a description for this new section and updated the value to use a #RRGGBBAA format.
@custom-media --viewport-small (min-width: 600px); | ||
@custom-media --viewport-small-max (max-width: 767px); | ||
@custom-media --viewport-medium (min-width: 768px); | ||
@custom-media --viewport-wpadmin (min-width: 783px); |
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.
We should keep the wpadmin breakpoint defined (even if we're using it for tablet). Often designs don't stick to that value when they get implemented.
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 removed this because it no longer is true and now aligns with the --mq-wp-medium
media query at the bottom of this file. Maybe the breakpoints have been updated in core, but 783px
is no longer mentioned in here: https://github.com/WordPress/gutenberg/blob/trunk/packages/base-styles/_breakpoints.scss.
I could rename medium
to be wpadmin
or adminbar
so it's more in line with what it's used for generally?
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.
Interesting. Let's go with adminbar
. We often use a different medium value on projects.
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.
Updated this variable to be --mq-wp-adminbar
. This breakpoint will match the defined tablet
media query but can be used in places where contexts dictates it makes more sense to reference the admin bar media query rather than the tablet media query.
@GeoffDusome , left a couple follow-ups to your questions. Feel free to merge after you've reviewed/updated. Thanks! |
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.
🥔 💨
What does this do/fix?
styles
directory, it's no longer necessary to define block variations in the PHP classes that extendBlock_Base
./styles
directory; I'm on the fence here about restructuring this directory to additionally house the block styles that live withintheme.json
currently (to be something likestyles/blocks
&styles/variations
). This would allow us to reduce the size of thetheme.json
file, but would split things up a bunch and possibly make finding things confusing.primary
,secondary
. I wasn't quite sure why design made this decision, but I'm 99% sure we wanted to get away from doing this. Please let me know if I'm just wrong here.--transition
.focus-visible
->/actions
; stacking order ->columns
block.mask
where possible so that we don't need to define multiple icon colors.scss
files in the theme should now more consistently use themed defined CSS properties, rather than WP preset properties.theme.json
QA
Links to relevant issues
Screenshots/video: