Add dark colors support to Cart & Checkout controls #2981
Conversation
senadir
added
focus: components
senadir
requested review from
haszari and
Aljullu
and removed request for
a team
|
Size Change: +1.95 kB (0%) Total Size: 1.66 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Looking good - this is a nice enhancement @senadir. I tested with customised dark-background Storefront and also with Twenty Seventeen which as a Colors > Dark option. I saw some little quirks here and there but no major blockers. Note I did my testing primarily in Safari / Firefox, and focused on the dark mode feature (not the other issues addressed in this PR).
Alongside a few code comments, I have a bunch of mostly minor feedback. Some might be better to address within this PR but much can be done as follow up. Let me know if you want me to make any follow up issues (I'll assume you're handling that).
- The UI & naming of the merchant/editor block option - probably good idea to loop in design and make any changes before merge. Also the block option is displayed at the top of the sidebar - I think it should be further down, it's more "advanced" than the others (e.g. address options).
- Consistent/clear approach to hard-coded vs colour variables. I like the idea of the colours being in variables, but I don't understand why some are variables and some are hard coded inline.
- No live preview in editor - maybe there's a technical blocker for this. It makes the editor option much more tricky for merchants, live preview would really help.
- Some form elements still white (this is probably follow up, as it's different in different themes).
- Order notes textarea - white in Storefront, dark in Twenty Seventeen.
- Stripe credit card fields - white in both themes I tested.
I see this PR has fixes for three issues – I can see why, since they can be somewhat tested together. However in general I'd prefer to have a PR for each issue to ensure they all get appropriate review, and if there are dependencies between PRs, then we can be explicit about that.
| @@ -8,6 +8,9 @@ | |||
| &:focus { | |||
| outline: 2px solid $core-grey-light-600; | |||
| } | |||
| .has-dark-styles &:focus { | |||
There was a problem hiding this comment.
Is the outline width different for dark mode - if so, what's the rationale for that?
There was a problem hiding this comment.
There's no design spec for this, I felt that 2px was too much, and 1px is too little for light mode, I switched back to 2px to align both styles.
|
Saw an issue in Twenty Seventeen with |
There was a problem hiding this comment.
This is looking great! 😍
I think @haszari has done a good review here, so just adding my two cents of some stuff that I saw:
- Color of text input placeholders should change in dark mode, otherwise contrast is too low:
- Radio inputs are still dark in the editor:
I think the best way to proceed here would be to refactor radio inputs CSS completely: we shouldn't have the frontend and editor styles duplicated with different specificity. That's not blocking and can be handled in a follow-up in my opinion.
senadir
force-pushed
the
add/dark-theme-support-to-cart-checkout
branch
from
8b520e5
to
5f8baf4
Compare
|
Just as a FYI (not a blocker to work being done here, but just for awareness), this came up in WordPress core-editor chat today: WordPress/gutenberg#24368 |
I can't reproduce this, are you sure? cc @haszari
Do you mean the heading element in 2017? |
Because they're disabled. cc @Aljullu |
senadir
force-pushed
the
add/dark-theme-support-to-cart-checkout
branch
from
5f8baf4
to
2816f72
Compare
There was a problem hiding this comment.
This is looking great and working really well, nice job @senadir !
Even with my terracotta storefront 🙌 :
One tiny thing to possibly change - the Dark mode inputs shouldn't be the top option in the editor IMO. Shipping, address, nav options are all more important to cart/checkout – I think it's safe to put the new setting at the bottom, above the feedback panel.
The Stripe credit card form field is still showing white, but possibly this can be handled as follow up (since it's somewhat out of our control).
| @@ -44,6 +54,8 @@ | |||
| } | |||
| } | |||
|
|
|||
| // Hack to hide the check mark in IE11 | |||
| // See comment: https://github.com/woocommerce/woocommerce-gutenberg-products-block/pull/2320/#issuecomment-621936576 | |||
There was a problem hiding this comment.
Is this comment still relevant? Not a blocker :)
There was a problem hiding this comment.
Not much, but it wasn't added on the previous PR and stayed here during rebase, no point in deleting it and putting it back in a new PR.
| export const PAYMENT_GATEWAY_SORT_ORDER = getSetting( | ||
| 'paymentGatewaySortOrder', | ||
| [] | ||
| ); |
There was a problem hiding this comment.
Interesting that this formatting snuck through!
| @@ -24,6 +25,10 @@ const blockAttributes = { | |||
| type: 'number', | |||
| default: 0, | |||
| }, | |||
| hasDarkControls: { | |||
| type: 'boolean', | |||
| default: HAS_DARK_EDITOR_STYLE_SUPPORT, | |||
There was a problem hiding this comment.
This might not be a big deal .. but I'm still a bit confused about the relationship between current_theme_supports( 'dark-editor-style' ) and our new block setting.
dark-editor-styleis an editor-focused option, informing Gutenberg that it should use dark mode to work better with the editor styles for the themehasDarkControlsis a front-end, shopper experience option
I think it's probably fine to use it as the default, but since they are quite different it would be good to have a comment explaining the intention with the default.
Here's my understanding of why we're using this as the default:
- If a theme is predominantly "dark", it's likely to want to opt-in to our dark mode for cart/checkout elements.
- Themes that have user-defined color options (e.g. Storefront) won't get the benefit of the default, because they won't declare "dark"ness, since it's customizer dependent.
There was a problem hiding this comment.
If a theme is predominantly "dark", it's likely to want to opt-in to our dark mode for cart/checkout elements.
Themes that have user-defined color options (e.g. Storefront) won't get the benefit of the default, because they won't declare "dark"ness, since it's customizer dependent.
Yes, we have this method to assume a theme is dark:
- The themes supports
dark-editor-style, by explicitly supporting this, most editor borders and placeholders will be white, making them illegible on white background, by this we can assume the background is dark.
A theme can also register support for custom-background, however I'm not aware if there's a standardized way of fetching that value, get_theme_support will only fetch the default value.
If we can fetch the custom-background value, we can run some calculations (like TwentyTwenty) to see if the background is dark or light, and set the default based on that.
| 'woo-gutenberg-products-block' | ||
| ) } | ||
| help={ __( | ||
| 'Inputs styled specifically for use on dark background colors.', |
| background-color: $input-background-dark; | ||
| border-color: $input-border-dark; | ||
| color: $input-text-dark; | ||
| } |
There was a problem hiding this comment.
❤️ how clean and simple all the new dark-mode rules are now - opt-in to alternative, standardised colours. 👌
Closes #2320
Fixes #2986
This PR introduces dark styles to our controls, it adds them to:
Screenshots
Dark mode (in storefront)
Light mode
It also introduces the option to toggle this mode or not, the default value of that attribute is inherited from the theme, themes can ask GB for dark mode support by defining
add_theme_support( 'dark-editor-style' ), we look for that and decide accordingly.By default, previous blocks will stay on light mode, even if the theme has dark styles enabled, if they resave the block again, or insert it again, the new default value will be inherited from dark styles option.
Accessibility
Up to the designs.
How to test the changes in this Pull Request:
Changelog