-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Add a Color Scheme setting for Menus-Header #2622
Add a Color Scheme setting for Menus-Header #2622
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.
Hey Louisa, everything looks good to me 👍, just left minor suggestions. It looks like you accidentally added whitespace.
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.
Found one hiccup with the gradient stuff but I think it's a quick fix. Otherwise just a question for @YoannJailin before we request translations.
locales/en.default.schema.json
Outdated
"content": "Color" | ||
}, | ||
"header_color_scheme": { | ||
"label": "Header color scheme" |
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.
@YoannJailin I don't feel strongly at all, but just wondering if we don't want to keep the original "Color scheme" label for the header's color scheme setting. I could certainly see this adding some clarity for users, but the setting is within the "Header" section as it is, so I somewhat feel like we don't need the additional qualifier.
We only have a couple other instances of multiple color schemes in one section to pull precedent from but they just use "Color scheme" to represent the section's color scheme.
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 i agree we can replace "Header color scheme" by "Color scheme". I would keep the second one named "Menu color scheme". @lougoncharenko @kmeleta
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 went ahead and changed it to display "Color Scheme" for the first one and left "Menu color scheme" for the second one
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.
This works, but I'd say we could probably clean things up a bit by removing the header_color_scheme
translation entry that we added altogether, and pointing our setting label back to the global string for "Color scheme" "label": "t:sections.all.colors.label",
like it was before. That we're we're not duplicating anything and there will be one less new string to translate. Let me know if that doesn't make sense.
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.
Changed it back to the original translation and removed the header_color_scheme translation
… global string for color scheme
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.
Looks good! Let's request translations.
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.
Looks good to me. I'll approve once translations are done 👍
* seperated color scheme for header and menu * made changes to remove white space as suggested from eugene * made some changes requested by ken * changed Header color scheme to Color scheme per yoann request * removed header_color_scheme translation and set the label back to the global string for color scheme * Update 15 translation files * Update 5 translation files --------- Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
* shopify/main: (59 commits) [Announcement bar] Add social icons (Shopify#2497) Update theme version to match the pubic release (Shopify#2698) Add release/v10.0.0 branch fixes to main (Shopify#2693) fix default UI for dropdown and mega menu (Shopify#2644) Fix link formatting in Related Products heading (Shopify#2680) Update 2 translation files (Shopify#2637) Enable gift card recipient form by default on featured product section (Shopify#2666) Gift cards/enable recipient form by default (Shopify#2618) Add a Color Scheme setting for Menus-Header (Shopify#2622) Made mobile drawer full width by default-header (Shopify#2625) Allow multiple announcement bars in Header group (Shopify#2619) [Feat Product] Add rating styling sheet (Shopify#2620) Fix password page variables (Shopify#2607) Fix transform applied when it should not for sliders (Shopify#2606) Modify info string for gift card recipient checkbox (Shopify#2588) [3D lift animation] Raise hovered card above adjacent cards (Shopify#2589) Remove fallback color scheme info text (Shopify#2602) Fix CSS specifity issue to cancel animation for theme editor events (Shopify#2605) Remove settings daya for icon color (Shopify#2601) Follow ups for accessibility of announcements slider (Shopify#2580) ...
* seperated color scheme for header and menu * made changes to remove white space as suggested from eugene * made some changes requested by ken * changed Header color scheme to Color scheme per yoann request * removed header_color_scheme translation and set the label back to the global string for color scheme * Update 15 translation files * Update 5 translation files --------- Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
PR Summary:
Added the ability to use a different color scheme for header and menu.
Why are these changes introduced?
Added a second color scheme for navigation, so that the header and navigation can use different color schemes.
Fixes #2565
What approach did you take?
Kept original
color_scheme
setting for header with updated label.And added new setting
menu_color_scheme
Added "color" header to header section schema instead of global translation
Visual impact on existing themes
Not applicable.
Testing steps/scenarios
Demo links
Checklist