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

[Image with text] Add color scheme picker for section wrapper #3016

Merged
merged 2 commits into from
Oct 5, 2023

Conversation

ludoboludo
Copy link
Contributor

@ludoboludo ludoboludo commented Sep 27, 2023

PR Summary:

Add color picker for the image with text section

Why are these changes introduced?

The image with text section has historically only offered a color picker for the content container and the media but not for the whole section.
This PR aim to change that and match the experience you're getting on the Multirow section where you have two color pickers.

What approach did you take?

Had to add some element to wrap the content that doesn't get the page-width class applied to it.

Visual impact on existing themes

This enables something new where the whole image with text section can get a background color scheme.

Testing steps/scenarios

  • Use the image with text section on different templates
  • With different color scheme backgrounds and color schemes for the content container
  • Test multirow as well with a gradient background on the section with a png image (the CSS styling impacts it)

Demo links

Checklist

Copy link
Contributor

@kmeleta kmeleta 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 good. Found a couple cases so far that we should try and clean up if we can, but most of the main use cases are accounted for I think.

assign fetch_priority = 'auto'
if section.index == 1
assign fetch_priority = 'high'
endif
if section.settings.color_scheme != section.settings.section_color_scheme
assign add_color_classes = true
Copy link
Contributor

Choose a reason for hiding this comment

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

The trouble with excluding the color classes entirely, rather than relying on the "fake" transparent effect of using the same color scheme classes is that is can't hide overlaps and shadows when needed. I know with the motion side-effects we want to need to rely on true transparency more though.

This one with the overlap layout I think we probably need to address.

Some others involving shadows like this have always been more edge casey so I wouldn't push back too hard on these regressions if our primary use cases are behaving correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think except the first one, I can replicate the rest of them on /main today 🤔 I pushed a fix for the first scenario, good catch 👍

@eugenekasimov eugenekasimov self-requested a review October 4, 2023 01:57
@eugenekasimov
Copy link
Contributor

Hey Ludo, I didn't have time today for detailed tests, I'll continue tmr. One thing, however, brought my attention. Won't we confuse users with swapping settings names? Before, the setting for the color scheme was named Color scheme and as you mentioned in the description it used to be changing just content and media. Now this setting changes the background and for the content and media there is a new name Container color scheme? 🤔 I was confused with that and I don't associate a name container with content but I may be wrong. It's just thoughts.

@ludoboludo
Copy link
Contributor Author

Hey Ludo, I didn't have time today for detailed tests, I'll continue tmr. One thing, however, brought my attention. Won't we confuse users with swapping settings names? Before, the setting for the color scheme was named Color scheme and as you mentioned in the description it used to be changing just content and media. Now this setting changes the background and for the content and media there is a new name Container color scheme? 🤔 I was confused with that and I don't associate a name container with content but I may be wrong. It's just thoughts.

YEah I agree, it's a little confusing. The reason I didn't change it is because this is the way it's labeled for the multirow section as well so I wanted to reuse the same pattern.

@melissaperreault
Copy link
Contributor

You had a good call @ludoboludo for keeping consistency between other section settings. We do have a theme setting category named "Content container" and we use "container" in various places to refer to the content box. Also, merchants have a direct view of where this setting will apply as they play with the options in the list. Not too concerned for now, maybe @katycobb can chime in for future iterations.

Copy link
Contributor

@kmeleta kmeleta left a comment

Choose a reason for hiding this comment

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

lgtm. I'm also in favor keeping the setting labels consistent with what we do on multirow, but would of course defer to Meli/Katy.

@katycobb
Copy link

katycobb commented Oct 4, 2023

Hey friends! Agree the labeling here isn't a dealbreaker to shipping.

My quick POV is that if there's just one color picker, call it Color scheme. But if we have more than one, we might want to add differentiators to both to describe what each applies to.

Here I suggest updating Color scheme to Background color scheme to be more specific about the difference between the two pickers. Container color scheme works, and is consistent with how we label other settings that apply to content containers.

Happy to do this (and multirow) in a followup though 👍

Copy link
Contributor

@eugenekasimov eugenekasimov left a comment

Choose a reason for hiding this comment

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

Everything looks good to me. I like a name suggestion Katy mentioned here, but it's not a blocker for me 👍

@ludoboludo ludoboludo merged commit 5c8c151 into main Oct 5, 2023
9 checks passed
@ludoboludo ludoboludo deleted the color-scheme-image-with-text branch October 5, 2023 12:36
@danielvan
Copy link
Contributor

Hey friends! Agree the labeling here isn't a dealbreaker to shipping.

My quick POV is that if there's just one color picker, call it Color scheme. But if we have more than one, we might want to add differentiators to both to describe what each applies to.

Here I suggest updating Color scheme to Background color scheme to be more specific about the difference between the two pickers. Container color scheme works, and is consistent with how we label other settings that apply to content containers.

Happy to do this (and multirow) in a followup though 👍

+1 here.

hz70ma added a commit to hz70ma/dawn that referenced this pull request Jan 31, 2024
* Update 1 translation file (Shopify#2997)

Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

* Update 1 translation file (Shopify#3001)

Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

* [Image_tag] Update to remove lazy loading and let it automatically decide the best loading strategy (Shopify#3002)

* remove lazy loading where necessary to better performance

* add fetch priority

* [Facets] update filter counts on filter selection (Shopify#2988)

* Correct CSS (Shopify#3003)

* Check if there is compare_at_price (Shopify#3000)

* Update 1 translation file (Shopify#3012)

Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

* [Facets] fix mobile count update (Shopify#3018)

* Update 1 translation file (Shopify#3043)

Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

* Update 1 translation file (Shopify#3044)

Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

* [Image with text] Add color scheme picker for section wrapper (Shopify#3016)

* [Image with text] Add color scheme picker for section wrapper

* address review comment about overlap and transparency

* Refactor `quick-order-list.css` (Shopify#3006)

* Fix Add to Cart error on page load/slower connections (Shopify#3008)

* [Facets] dynamic header (Shopify#3048)

* Add component-card.css to cart drawer (Shopify#3049)

* [Refactoring] Replace loading spinner with snippet (Shopify#2996)

* Change spinner for main-cart-items

* Change spinner for cart-drawer

* remove extra div and change classes

* Replace spinner with snippet for pdp and fead prod

* Remove unnecessary classes

* Add comment for snippet

* Change naming for files and css classes

* Put back deleted elements

* Remove unused classes

* Put back loading-overlay for cases when no spinner

* Chnage loading-overlay--error name

* Minor change in CSS

* Move loading-overlay styles to template-collection

* Rename component-loading-overlay

* Fix bug with overlay and spinner for facets

* Remove component-loading-spinner import from files

* Use spinner snippet for predictive search

* Hide product count when loading for drawer filters

* Address feedback. Clean-up.

* Minor changes to address feedback

* Fix spinner conflict with cart drawer

* Address feedabck

* Add missing whitespace for if tag

* [Sliders] Regression fix. Apply CSS only when necessary in theme editor (Shopify#3070)

* [Sliders] Regretion fix. Apply CSS only when necessary in theme editor

* change of approach

* add comment to explain CSS

* Add visual representation for filters (Shopify#3045)

* [Facets] update visual representation of facets operators (Shopify#3061)

* [Collection template] Product grid color scheme picker (Shopify#3017)

* [Collection template] Product grid color scheme picker]

* move color setting up in the list

* support gradient

* [Cart] Add color picker on cart page and in general cart settings (Shopify#3021)

* rebase final final

* use the update for the loading spinner

* add color picker for the cart drawer and cart popup

* fix spacing issues on cart page

* add gradient support

* address review comments

* adjust where the class is added. Fix gradient issue

* isolation needed for shadow

* add comment

* Remove unnecessary isolate

* use the isolate class instead

* [Product] Add color scheme picker (Shopify#3015)

* [Product] Add color scheme picker

* support gradient

* apply background color to be full width

* address review comments: color scheme applied to quick add modal and lightbox modal

* add color scheme to product availability drawer and move color settings up

* fix color classes to work properly with gradients

* Add support for gradient on modal

* remove console log

* [VisualDisplay] bump the active outline width (Shopify#3083) (Shopify#3091)

* 12.0.0 Version Bump and release notes (Shopify#3092)

* Update 1 translation file (Shopify#3093)

* updated code to match new color scheme naming (Shopify#2801)

* updated code to match new color scheme naming

* removing additional background-1 after rebase

* Fixed race condition for cart note updates (Shopify#3125)

* [Facets] support dynamic facet lists (Shopify#3123)

* Assign font family to input fields (Shopify#2871)

* Price per item, Popover and global style bugs (Shopify#2851)

* Fix cart submission on Quick Order List (Shopify#2868)

* Social icons: Visual fixes (Shopify#2855)

* Adjust spacing.

* Facebook + Tumblr icon size adjustments.

* Update social icon SVGs.

* Tidy up new icon sizes again.

* Resize icons.

* Make spacing slightly smaller.

* Make icons larger so they're more similar to the old sizes.

* Remove padding to compensate for extra viewbox space.

* Try a smaller Twitter icon.

* Update snapchat icon.

* Resize social links in menu drawer to 44x44

* replace translation string to have the translation visible (Shopify#2869)

* B2B compare at price with price range (Shopify#2858)

* Add sale badge and price-range for volume-pricing

* Add compare_at price to PDP and Feat Prod.

* Change opacity to 100% for price per item.

* Update the logic

* Hide price per item for unavailable variants.

* Remove margin for dl.

* Refactoring

* Correct a mistake in liquid.

* Change the JS logic back for updating price per item

* Add compare at to prod card. Add style to compare at

* Assign font family to input fields.

* Update assets/base.css

Co-authored-by: Kai <KaichenWang@users.noreply.github.com>

* Use the theme's font style + weight in form elements.

---------

Co-authored-by: Sofia Matulis <sofiamatulis@users.noreply.github.com>
Co-authored-by: melissaperreault <melissa.perreault@shopify.com>
Co-authored-by: Eugene Kasimov <105315663+eugenekasimov@users.noreply.github.com>
Co-authored-by: Kai <KaichenWang@users.noreply.github.com>

* Update 1 translation file (Shopify#3155)

Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

* Update 1 translation file (Shopify#3157)

Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

* Update 1 translation file (Shopify#3158)

Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

* Update 1 translation file (Shopify#3161)

Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

* Update 2 translation files (Shopify#3160)

Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

* Applied image shape and ratio to placeholder images (Shopify#2817)

added classes and styles to get image shape and image ratio working

added styling on portrait placeholders

aligned placeholder when in potrait mode

applying code review suggestions and removed image-ratio

* [Visual Display] Display accurate filter colors when high contrast mode is enabled (Shopify#3165)

* Improved country selectors (Shopify#3175)

Original PR with review comments -> Shopify#3135

* Update inline quantity error styles. (Shopify#3150)

* Update 1 translation file (Shopify#3177)

Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

* [Variant Picker] Update settings copy (Shopify#3173)

* Bring back the lighthouse-ci-action to v1 (Shopify#3181)

This will make sure new releases are automatically used.

Better for forks as well.

* Changed slider to work on tablet for multicolumn (Shopify#3176)

* Changed slider to work on tablet for multicolumn

* Adjusted to prevent early cutoff on tablet

* Adjusted Featured Collection placeholders to work with any number of desktop columns (Shopify#3182)

* Adjusted featured collection placeholders to work with any number of desktop columns

* [Variant Picker] Add swatch display type (Shopify#3180)

* [VariantPicker] Unify variant selects and radios under new component

* [VariantPicker] Add swatch settings

* [VariantPicker] Move styles to dedicated CSS file

* [Variant Picker] Update settings copy

* [VariantPicker] Add swatch component

* [VariantPicker] Add a swatch to selected dropdown option

* [Variant Picker] Update disabled state

* [Variant Picker] Swatch snippet

* [Variant Picker] Swatch input snippet

* [Variant Picker] Tweak swatch border colors

* Update swatch border (Shopify#3184)

* Update translations: merchant (Shopify#3178)

* Update 1 translation file

* Update 1 translation file

* Update 2 translation files

* Update 1 translation file

---------

Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

* [Variant Picker] Ensure that swatches wrap correctly (Shopify#3185)

* Focus search on country selector open and fix iOS bug (Shopify#3183)

* Focus search on country selector open

* Set stacking context

* Prevent sticky header from hiding when country selector is open (Shopify#3188)

* change to 100% (Shopify#3190)

* [Variant Picker] Simplify swatch settings (Shopify#3189)

* [Variant Picker] Simplify swatch settings

* Update 7 translation files

* Update 12 translation files

* Update 1 translation file

---------

Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

* Escape filter label consistently (Shopify#3192)

* Update 1 translation file (Shopify#3202)

Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

---------

Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
Co-authored-by: Ludo <ludo.segura@shopify.com>
Co-authored-by: Patrick Racicot <patrick.racicot@shopify.com>
Co-authored-by: Eugene Kasimov <105315663+eugenekasimov@users.noreply.github.com>
Co-authored-by: Andrew Etchen <andrew.etchen@shopify.com>
Co-authored-by: Jason Addleman <jason.addleman@shopify.com>
Co-authored-by: Sofia Matulis <sofiamatulis@users.noreply.github.com>
Co-authored-by: Louisa Goncharenko <93098869+lougoncharenko@users.noreply.github.com>
Co-authored-by: Tyler Alsbury <60230011+tyleralsbury@users.noreply.github.com>
Co-authored-by: Kjell Reigstad <kjell@kjellr.com>
Co-authored-by: melissaperreault <melissa.perreault@shopify.com>
Co-authored-by: Kai <KaichenWang@users.noreply.github.com>
Co-authored-by: Alex Ilea <6627543+alisterdev@users.noreply.github.com>
Co-authored-by: Abdulrahman Hamideh <abdulrahman.hamideh@shopify.com>
Co-authored-by: CP Clermont <cp.clermont@shopify.com>
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
…y#3016)

* [Image with text] Add color scheme picker for section wrapper

* address review comment about overlap and transparency
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants