-
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 new text and overlay settings to image with text #944
Conversation
2f2cfe8
to
c6429d6
Compare
fb8756c
to
021ca49
Compare
021ca49
to
4318001
Compare
Updates
|
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.
First pass looks good. There's a minor spelling issue, but testing + code look great. Will wait for UX to review and give feedback before finalizing my review.
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 very cool. I like this new flexibility and cool looking design with the overlap 👌
|
||
.image-with-text--overlap .image-with-text__text-item { | ||
display: flex; | ||
padding: 3rem 0; |
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 might want to remove this padding only at the top, only at the bottom or both depending on the desktop content position
: screenshot.
Since we already have the margin on the section at the top and bottom.
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.
locales/en.default.schema.json
Outdated
@@ -958,7 +958,21 @@ | |||
"options__3": { | |||
"label": "Large" | |||
}, | |||
"label": "Image ratio" | |||
"label": "Image height", | |||
"info": "Determines section height." |
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.
Here if the text box is long due to content it makes the image adapts to the space its given. So say you have chosen adapt to image
and the content is long, then your image isn't fully shown anymore (depending on the aspect ratio).
Should it keep it's aspect ration and be aligned vertically or we keep it as is ? I think this behaviour is similar for another section where there is a min-height set. So it won't technically, always be Adapted to the first image
.
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 is expected, and it already behaves this way. It's based on the image aspect ratio. If we always want the full width of the image to be visible, then the entire section would need to be revisited because the section's behaviour (no-overlap) is based on the image being the same height as the text. I'll leave this as is, and talk to Wik about this as a follow up.
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.
Ah sounds good 👍
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.
Does "Adapt to content" or something similar make more sense? The behaviour looks solid but the content might be a bit misleading. No strong opinions.
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 don't think it's adapting to content though. So the height of the image is based on the image height setting - and in this case it's adapting to the image height, but the width is determined by another setting.
"options__2": { | ||
"label": "Overlap" | ||
}, | ||
"label": "Content layout" |
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 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 like this suggestion - being able to choose how they layer can unlock new effects.
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 will add this as something to consider as a follow up. Particularly because we are not offering a way to modify the overlap value and it could result in text being covered.
"label": "Overlap" | ||
}, | ||
"label": "Content layout" | ||
}, |
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 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 would be sick but to really do it properly, we would need to have some interface (similar to gradients) in the editor for creating these shapes.
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.
Yeah for sure. Though it seems that what triggers the change is to have a rough implementation of it first maybe 🤔 But it doesn't make for the best experience for merchants.
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.
The neat thing too is that you can do clip path based on an svg path. And have even cooler effects: blobs, dots patterns, etc. There are some cool example in Sara Soueidan's article.
-The Desktop Content Position setting seems to be moving the image, not the text/button/container. I think this is opposite behavior to other sections like slideshow? Note: this only happens when I choose “Small” for image height. -Question: what is the default behavior on mobile for Content Position? Since there isn’t a mobile-specific setting, we might want to add help text under Desktop Content Position to describe what to expect on mobile. (Following the same pattern of the Desktop image placement and Desktop image width settings) |
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.
Approving. Let me know if it goes stale - everything looks good to me.
config/settings_data.json
Outdated
@@ -60,4 +60,4 @@ | |||
} | |||
} | |||
} | |||
} | |||
} |
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.
Not sure when or where it was removed, but we should keep the trailing new line to prevent conflicts in this file in the long term.
} | |
} | |
templates/index.json
Outdated
@@ -103,4 +103,4 @@ | |||
"video", | |||
"multi_column" | |||
] | |||
} | |||
} |
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.
Same deal here.
} | |
} | |
@martinamarien just jammed with Wik - we'd like to add help text under the Let me know if you need more info! cc @wiktoriaswiecicka |
👋 Not sure if this is relevant to this PR, it can be tackled on another one.
|
e48ac93
|
||
.image-with-text--overlap .image-with-text__text-item { | ||
display: flex; | ||
padding: 3rem 0; |
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.
} | ||
|
||
.image-with-text--overlap .image-with-text__content { | ||
transform: translate(0 , -3rem); |
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 I think creates some extra space on mobile. Seems like using a negative margin-top makes it better: video
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. Tested on iOS as well and no issues noticed.
I wonder if a medium
size option should be added for the Image height
settings 🤷 but that can easily be added in the future if needed.
I think it could be useful when the image you're dealing with is portrait and long. You don't end up having an option that is between small and large since the aspect ratio makes adapt to image
be as large as the large
option or bigger.
* Add new settings and translations * add text overlay, image and text alignment css * remove specificity and update layout setting * update content * update label * Add caption-with-letter-spacing and fix translation content * update color behaviour and add subtitle setting * fix center spelling * Update 11 translation files * Update 15 translation files * address feedback and fix content * Update 5 translation files * Update 15 translation files * Update caption block styling * Update 3 translation files * Update 18 translation files * Update 2 translation files * re-add new line Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
Why are these changes introduced?
This PR adds new settings to the image with text section:
fixes #945
What approach did you take?
1. Horizontal Text alignment (mobile and desktop)
Used
text-align
to change text position.2. Vertical text alignment
Used flexbox for the vertical alignment of the text inside of the standard layout and for the overlap layout.
3. Overlay text box
I decided not to use
position: absolute
for this layout. The reason being: if the text were to expand below or above the image, it would overlap content above/below it. Instead, I used flexbox andheight: auto
to allow text box to collapse.Behaviour of text box when content position is set to:
3rem
below the top of the image, and the textbox will grow downward on resize or if the text is larger than the image height.3rem
above the bottom of the image, and the textbox will grow upward on resize or if the text is larger than the image height.4. Image size
Leveraged our grid system and flexbox to ensure image/text grows as needed to take up remaining 2/3 of the space for the
small
andlarge
settings.Demo links
See demo shop
Checklist