-
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
Adding colour scheme settings to various sections #1034
Conversation
81681b3
to
f5f0359
Compare
59a3dda
to
8aaff51
Compare
1710f12
to
6f152f4
Compare
6f152f4
to
ecab9d7
Compare
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.
Looking good 👌 Noticed a few small things.
I think it needs to be also added to the contact form
section. And maybe they announcement bar as well ? screenshot
Should the video section have it as well 🤔 It only would come in handy when using a vimeo video and if the image is a different aspect ratio than the video. screenshot
Also I feel like the position of the setting can vary a little from section to section. Like header and footer have it as the first option but the rest of the section have it somewhere else.
Each individual announcement bar block does have a colour scheme setting, so I think we're good for those. |
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.
Looking good! Just noted a couple things
sections/featured-product.liquid
Outdated
@@ -30,7 +30,7 @@ | |||
<link id="ModelViewerOverride" rel="stylesheet" href="{{ 'component-model-viewer-ui.css' | asset_url }}" media="print" onload="this.media='all'"> | |||
{%- endif -%} | |||
|
|||
<section class="{% if section.settings.secondary_background %} background-secondary{% endif %} color-{{ section.settings.color_scheme }} gradient"> | |||
<section class="{% if section.settings.secondary_background %}background-secondary{% else %}gradient{% endif %} color-{{ section.settings.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.
There are still a few things to figure out I think with how show seconday background
should work with the color scheme chosen 🤔
screenshot
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.
Alright, stuck the class further down the chain - it was overriding the foreground colour with white which was causing the issue. 🤦
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 were going to remove the secondary background and add a Container color scheme
picker here similar to collapsible content. If that isn't possible to do right now, the secondary background should take on the text colour used in the colour scheme picker. So if the secondary background is 4% of text when set to background 1, it should be 4% of the button label when set to accent 1. Does that work as a solution? @tyleralsbury Happy to pair if needed
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 see - okay, I think that's the behaviour we had before because it was giving it a white background when the text was white. I'll do it that way and we can test to ensure that it's what we're looking for.
For now, we can keep the secondary background because adding a second picker adds complexity.
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.
Okay, I think I've got it working the way we expect it to.
- If secondary background is not selected, it gets a full screen background with the same colour as the internal container
- If it is selected, the text colour is taken (either the actual text colour for background, the background colour for inverse, or the button outline colour for accent)
Should be working like that now - I also fixed the gradient so that it works in both cases.
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.
Should we also add it to the Video section, Slideshow (for when it's using the grid layout) and maybe Custom liquid as well? 🤔
Should collection list also get that setting (not the section) ? 🤔 Since on the collection page we can do it for the header and product grid, I'm wondering if that'd make sense to had it there too.
But then that'd mean adding it to the templates like page, product page, etc. So maybe I'm overthinking it and we can look into this later 😅
Other than this ^ and what I mentioned in my couple comments, things look good 👌
Looking good!
I know some of the changes may be complex to change, so I'd be happy to pair and help find a solution that works. Thanks @tyleralsbury ! |
It shouldn't be on the product grid because then we would have to add it to all templates like you've specified. We opted not to add it to the slideshow and video since they are full width and it would add unnecessary complexity for video. Also, the slideshow has a colour picker for the text box. Interesting point about custom liquid. Do either of you have any thoughts? I'm not too sure of the use cases @ludoboludo @tyleralsbury |
Probably, but I think we should do them as a follow-up in the interest of time. |
I actually did add it to the product grid because otherwise you can't have a custom colour for the banner that matches the rest of the page. I also did the same for the cart page so you can have a more branded cart. We can definitely add it to other templates as well, but I would prefer for us to do a proper pass and handle it as a follow-up severity 2 issue. Many of them have pretty simple implementations - the toughest one (as we've seen with featured product) is the product page itself. |
Custom Liquid is a bit of a weird one. I'd go with not including it there simply because the person adding in the content can kinda do whatever they want with it - so if they want to add colours, they can just add the class directly themselves. Custom Liquid is somewhat of a blank slate. On the other hand, we did add the padding setting there, so we could add this too so give them some controls that don't require code. |
Should we remove them? I can, but if we are going to eventually add them then should we keep them and just add to more template sections in the future as iterations? |
This is beyond what I'd consider to be a quick win. Can we instead keep it as-is and remove the "as section background" option from the dropdown. That will still allow them to have a secondary colour for the columns themselves and also to remove the secondary background entirely. |
This is done in #1090
I think this is also as expected in #1090 , is that right @ludoboludo |
7905581
to
c1ef7b3
Compare
I believe |
4d42e6f
to
b881d9c
Compare
@KaichenWang @ludoboludo Rebase is complete. |
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.
Looking good. 👍
I still think we should add it to the video section though 🤔 The section can be full width or not. So it would make sense to see it as an option. Otherwise it just looks like we missed it.
cc: @Oliviammarcello
Though maybe if we decide to add it to the video section we can do it as a follow up as we will need to request translation and that might delay when the PR will be merged. |
Nah, I've added a new key for translations that are shared throughout the theme - I didn't adjust the keys for every instance of the colour schemes, but I've got stuff like |
@ludoboludo @Oliviammarcello I added it to video. Was ezpz. |
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.
🎉
bebca9b
* Color scheme in sections
Why are these changes introduced?
Fix #966
What approach did you take?
For the sake of simplicity, we are applying this to each section in mostly the same way - generally by adding a wrapping element that has
color-{{ section.settings.color_scheme }} gradient
and adding it to the schema.Other considerations
There may be more nuanced approaches that could be taken for some sections / other sections that allow for more tweaking to happen on block levels, etc... but this initial implementation is meant to simply add flexibility to the ones that are the quickest and easiest to add the setting to.
Demo links
Checklist