Skip to content
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 gradient background settings #482

Merged
merged 29 commits into from
Aug 31, 2021
Merged

Add gradient background settings #482

merged 29 commits into from
Aug 31, 2021

Conversation

KaichenWang
Copy link
Contributor

@KaichenWang KaichenWang commented Aug 25, 2021

Why are these changes introduced?

  • Enable support for gradient backgrounds

What approach did you take?

  • Add theme settings to allow gradient (CSS background property)
  • Add gradient settings (type text) under Theme settings > Theme > Colors and pair with their respective "fallback" color settings:
    • Background 1
    • Background 2
    • Accent 1
    • Accent 2
  • Integrate gradient setting into existing color system
  • Introduce .gradient utility class. When applied to an element, that element will be styled with a gradient background (if the gradient setting is not blank, otherwise the fallback solid color is used).

Other considerations

Demo links

Checklist

@@ -2,9 +2,6 @@
"sections": {
Copy link
Contributor Author

@KaichenWang KaichenWang Aug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • revert before merge

@@ -1,63 +1,72 @@
{
Copy link
Contributor Author

@KaichenWang KaichenWang Aug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • revert before merge

bertiful
bertiful previously approved these changes Aug 25, 2021
assets/base.css Outdated
.gradient {
background: var(--color-background);
background: var(--gradient-background);
background-repeat: no-repeat;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this property (and this property in other files) needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, it's probably best to not make assumptions about what background-repeat value the merchant wants to use. I will remove this.

@Guillaumegranger1
Copy link

Guillaumegranger1 commented Aug 25, 2021

  • The color schemes doesn't seem to all be accurate. The inverse option should apply the Text color to the background and Background 1 solid to foreground elements. Screenshot. Here's an example where the Inverse scheme is working as intended: screenshot
  • I just saw another element that has the solid fallback when it shouldn't. It's the share button. Screenshot
  • There's also a hardcoded color I spotted on the copy icon, if we can fix it in this PR? Screenshot
  • The ring on qty inputs in the cart seems inconsistent with how the solid color shows up. Screenshot
  • Something happened to the number in the cart badge. It doesn't leverage the proper color, which I think should be the Solid button label. Screenshot

@KaichenWang
Copy link
Contributor Author

KaichenWang commented Aug 25, 2021

@Guillaumegranger1 Thanks for reviewing. I addressed the issues with ✅ and pushed the updates to the demo store. I also added a color_scheme setting for header as discussed. For the remaining items:

There's also a hardcoded color I spotted on the copy icon, if we can fix it in this PR?

Still looking into this, but I see different colors

Screen Shot 2021-08-25 at 4 24 00 PM

The ring on qty inputs in the cart seems inconsistent with how the solid color shows up.

Could you elaborate a bit more on what the issue is? Our implementation for focus rings uses the solid background color and is therefore visible on gradient backgrounds

@Guillaumegranger1
Copy link

• Regarding the ring, this might be fine after all. I wanted to make sure it was working as intended and consistent everywhere 👍
• Regarding the copy-paste icon in the share UI, I would have expected it to be 1 color only, and without background. I think it is a mistake the way it's been exported/implemented. Here's a .zip with a new export. Please @KaichenWang let me know if it helped.
icon-copy-paste.zip

@KaichenWang
Copy link
Contributor Author

@Guillaumegranger1

Regarding the copy-paste icon in the share UI, I would have expected it to be 1 color only, and without background.

I've pushed an update which removes the button background and updates the icon to the correct foreground color

Screen Shot 2021-08-25 at 6 02 49 PM

@KaichenWang
Copy link
Contributor Author

KaichenWang commented Aug 25, 2021

@Guillaumegranger1 , Another design decision is that now the header has been given a color_scheme setting, some other elements that are part of header have inherited the same color scheme. This includes:

These can also be set to always follow background-1 solid color instead. Please let me know what would make most sense

@Guillaumegranger1
Copy link

Guillaumegranger1 commented Aug 25, 2021

Another design decision is that now the header has been given a color_scheme setting,

@KaichenWang Pretty cool!! 😄 It behaves exactly how I like. I think it's perfect that the navigation elements and cart notification inherit the solid fallback.

  • Let's make sure the default header color scheme is set to Background 1

  • And then the only remaining thing that I see missing is the number in the cart badge. If I'm not mistaken, its color should be as follow:

• Accent 1: Accent 1
• Accent 2: Accent 2
• Background 1: Solid button label
• Background 2: Background 2
• Inverse: Text

@Guillaumegranger1
Copy link

And then the only remaining thing that I see missing is the number in the cart badge. If I'm not mistaken, its color should be as follow:

• Accent 1: Accent 1
 • Accent 2: Accent 2
• Background 1: Solid button label
• Background 2: Background 2
• Inverse: Text

☝️ I'm testing the demo regarding the last point and it seems to be fixed now. Screen recording

@KaichenWang are we all set?

bertiful
bertiful previously approved these changes Aug 30, 2021
@ludoboludo ludoboludo self-requested a review August 30, 2021 16:17
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty cool! We're going to see some interesting backgrounds over time I think (CSS pattern gallery)

A couple questions:

  • product modals don't have the gradient as a bg, is that intentional? I could see how we don't want to have them everywhere.
  • color scheme for the image banner isn't pulling from the gradient, should it ?

{
"id": "gradient_accent_1",
"type": "color_background",
"label": "t:settings_schema.colors.settings.gradient_accent_1.label"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like linking to some info about how gradients work or what to input could be useful. I see there is a placeholder in the popup but maybe we can link to the docs once they're out or confirm the url for them. Color scheme docs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. There's still some ongoing discussion regarding merchant-facing copy

sections/header.liquid Outdated Show resolved Hide resolved
sections/image-with-text.liquid Outdated Show resolved Hide resolved
sections/rich-text.liquid Outdated Show resolved Hide resolved
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And then on collage, the placeholder for the collection has a background set to it, so it looks a bit different than the other two cards:

Which highlights the fact that apparently the parent is a bit wider than the actual image: screenshot but that can be a follow up PR/issue

assets/base.css Show resolved Hide resolved
Co-authored-by: Ludo <ludo.segura@shopify.com>
KaichenWang and others added 2 commits August 30, 2021 15:03
Co-authored-by: Ludo <ludo.segura@shopify.com>
Co-authored-by: Ludo <ludo.segura@shopify.com>
@KaichenWang
Copy link
Contributor Author

KaichenWang commented Aug 30, 2021

@ludoboludo

And then on collage, the placeholder for the collection has a background set to it, so it looks a bit different than the other two cards

Collage section

The following blocks have a color-scheme setting and therefore have a background that is not identical to the page background:

  • Collection
  • Image

The other two types of blocks do not have a color-scheme setting and therefore inherit the page's background:

  • Product
  • Video

Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@@ -11,9 +11,15 @@
"label": "Accent 1",
"info": "Used for solid button background."
},
"gradient_accent_1": {
"label": "Accent 1 gradient"
},
Copy link
Contributor

@ludoboludo ludoboludo Aug 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The look of it is a bit cluttered. Would be nice to have some kinda of hierarchy/grouping. But for future polish/improvement. Also it looks like the look of the settings has been updated 🤔 (not here but on the theme editor level)
screenshot

@@ -724,6 +736,24 @@
"header": {
"name": "Header",
"settings": {
"color_scheme": {
"options__1": {
"label": "Accent 1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not urgent: there's quite a few instances of these colours throughout Dawn and each setting has its own translations—we can clean this up by referencing the same translation.

@KaichenWang KaichenWang merged commit 997f2a1 into main Aug 31, 2021
@KaichenWang KaichenWang deleted the add-background-setting branch August 31, 2021 03:30
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
* Add settings for gradients as text. Integrate setting into color system

* Set giftcard number bg to transparent

* Update temporary settings

* Update from Shopify for theme dawn/add-background-setting

* Update from Shopify for theme dawn/add-background-setting

* Update from Shopify for theme dawn/add-background-setting

* Update from Shopify for theme dawn/add-background-setting

* Update from Shopify for theme dawn/add-background-setting

* Update from Shopify for theme dawn/add-background-setting

* Remove background-repeat property

* Fix color-inverse color scheme. Fix cart icon bubble colors

* Add color scheme setting for header

* Update from Shopify for theme dawn/add-background-setting

* Update share button colors

* Update from Shopify for theme dawn/add-background-setting

* Update from Shopify for theme dawn/add-background-setting

* Update from Shopify for theme dawn/add-background-setting

* Update from Shopify for theme dawn/add-background-setting

* Update from Shopify for theme dawn/add-background-setting

* Update to color_background setting type for gradient colors

* Revert 8046f7d

* Revert setting changes from Shopify platform

* Update sections/rich-text.liquid

Co-authored-by: Ludo <ludo.segura@shopify.com>

* Update sections/image-with-text.liquid

Co-authored-by: Ludo <ludo.segura@shopify.com>

* Update sections/header.liquid

Co-authored-by: Ludo <ludo.segura@shopify.com>

* Set transparent background color to share

Co-authored-by: shopify-online-store[bot] <79544226+shopify-online-store[bot]@users.noreply.github.com>
Co-authored-by: Ludo <ludo.segura@shopify.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants