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 setting updates #2148

Merged
merged 6 commits into from
Dec 19, 2022
Merged

Image setting updates #2148

merged 6 commits into from
Dec 19, 2022

Conversation

kmeleta
Copy link
Contributor

@kmeleta kmeleta commented Nov 28, 2022

PR Summary:

Makes minor adjustment to image size settings for slideshow, image banner, and image with text.

Important: Removed a setting in image banner.

Why are these changes introduced?

Fixes #2133

What approach did you take?

  • Slideshow: changed default value for Slide height setting from Adapt to Medium
  • Image banner: Replaced Adapt section height to first image size checkbox setting with a new Adapt to first image option for existing Banner height select setting
  • Image with text: Added new Medium option to existing Image height select setting

Visual impact on existing themes

Slideshow: Default slide height has changed. This could cause a visual change for sections that don't have a value for this setting defined, but this shouldn't be the case.

Image banner: The setting changes in image banner should be considered a breaking change. The same visual result is still achievable with the new option added to the banner height setting, but sections will need to be manually updated to use the new setting.

Testing steps/scenarios

  • Ensure setting options and defaults match description above and in the linked issue
  • Verify new height setting for image with text works as expected
  • Verify no unexpected regressions in updated sections
  • Confirm all visual/breaking changes are accounted for in PR description

Demo links

Checklist

@kmeleta kmeleta force-pushed the phase2-image-setting-tweaks branch 2 times, most recently from c318ff3 to 8f8f3d9 Compare November 28, 2022 23:48
@kmeleta kmeleta marked this pull request as ready for review November 28, 2022 23:48
@eugenekasimov eugenekasimov self-requested a review November 30, 2022 17:57
@LucasLacerdaUX LucasLacerdaUX self-requested a review December 1, 2022 15:44
Copy link

@duygukalaycioglu duygukalaycioglu left a comment

Choose a reason for hiding this comment

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

Looking good! 👍

eugenekasimov
eugenekasimov previously approved these changes Dec 2, 2022
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.

I think you could use the Prettier plugin even though I haven't merged it yet.

The rest looks great!

LucasLacerdaUX
LucasLacerdaUX previously approved these changes Dec 8, 2022
Copy link
Contributor

@LucasLacerdaUX LucasLacerdaUX left a comment

Choose a reason for hiding this comment

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

Works as expected! Just left two minor comments before you request translations 👍

Comment on lines -166 to -196
"label": "t:sections.image-banner.settings.adapt_height_first_image.label",
"info": "t:sections.image-banner.settings.adapt_height_first_image.info"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove these translation keys in en.default.schema.json?

Copy link
Contributor Author

@kmeleta kmeleta Dec 19, 2022

Choose a reason for hiding this comment

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

I actually did mean to initially. Do you know what happens if we do remove some? Do the generated strings end up getting removed from the other files async or anything or would we have to go and specifically delete them ourselves?

If there's no conflicts or anything I might just clean this up in another image with text related PR I have going so I don't have to pull this down again, if that's ok with you.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure it gets removed by the translation platform at some point if we remove it from the English file. But I'm okay with doing it on a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey team! Seems like the translation was not removed. TIL!
There is an open PR for removing it from our community #2482

@kmeleta kmeleta force-pushed the phase2-image-setting-tweaks branch from 0da5add to cdbb6b3 Compare December 13, 2022 21:32
@kmeleta kmeleta mentioned this pull request Dec 14, 2022
8 tasks
Co-authored-by: Lucas Lacerda <37168033+LucasLacerdaUX@users.noreply.github.com>
Copy link
Contributor

@LucasLacerdaUX LucasLacerdaUX left a comment

Choose a reason for hiding this comment

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

👍

@kmeleta kmeleta merged commit 8aac1e5 into main Dec 19, 2022
@kmeleta kmeleta deleted the phase2-image-setting-tweaks branch December 19, 2022 21:35
TimmersThomas added a commit to TimmersThomas/shopify-template-houseofchocolate that referenced this pull request Dec 28, 2022
* shopify/main: (57 commits)
  Change sticky header settings (Shopify#2165)
  Image setting updates (Shopify#2148)
  Update 1 translation file (Shopify#2183)
  Update 2 translation files (Shopify#2182)
  Fix mega-menu vertical shadow issue (Shopify#2147)
  Update translations: buyer (Shopify#2180)
  Change product recommendations name to related products (Shopify#2161)
  Update 2 translation files (Shopify#2178)
  Add default text for cart-notification link (Shopify#2167)
  Update 1 translation file (Shopify#2177)
  Update 20 translation files (Shopify#2176)
  Update 1 translation file (Shopify#2174)
  Updates to collage settings and defaults (Shopify#2150)
  Update 1 translation file (Shopify#2173)
  Update issue templates
  Update layout label (Shopify#2164)
  Search ux improvements (Shopify#2127)
  Prettier all liquid files [part 2] (Shopify#2126)
  [Footer] Add 'Show policy links' setting (Shopify#2084)
  [Header] Move 'Logo' to Global Theme settings (Shopify#2083)
  ...
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
* Update height settings for image banner, slideshow, image with text

* Setting schema updates

* Update 17 translation files

* Update 2 translation files

* Update 1 translation file

* Fix formatting

Co-authored-by: Lucas Lacerda <37168033+LucasLacerdaUX@users.noreply.github.com>

Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
Co-authored-by: Lucas Lacerda <37168033+LucasLacerdaUX@users.noreply.github.com>
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.

[Media sizing Phase 2] Quick wins: Slideshow, image banner, image with text
6 participants