Card image border radius #1764

allylane · 2022-06-02T15:49:05Z

Fixes this issue
Apply media settings to images in Multicolumn.

There's currently no way of applying borders, border radius, or shadows to images in multicolumn layouts. This PR introduces them through media settings.

What approach did you take?
I applied the variables for media settings to .multicolumn-card__image, and I overrode border radius for .multicolumn-card__image if image ratio was set to circle

Other considerations
Multicolumn is sometimes used for Logos. If the theme has media borders, the logos will get those borders which may be less desirable for that specific scenario.

Testing steps/scenarios

Add an image to a multcolumn
Change Media settings

Have card images inherit styles from media.

melissaperreault

This is great thanks for taking this initiative for fixing it! This would solve this following issue #1525, if you can add it to the PR description! 🙏

I think we should still apply shadow for the Circle image ratio because otherwise it won't be predictable for merchants. We've been avoiding this kind of magic where merchants don't have control and want to rely on other ways to invest these kind of exceptions to the rule.

While testing I uncover:

Shadow gets cut off I'll investigate further Monday!

allylane · 2022-06-07T15:31:01Z

@melissaperreault I know what's causing that. I might have to rethink the approach to avoid it.

allylane · 2022-06-13T19:24:36Z

This is great thanks for taking this initiative for fixing it! This would solve this following issue #1525, if you can add it to the PR description! 🙏

I think we should still apply shadow for the Circle image ratio because otherwise it won't be predictable for merchants. We've been avoiding this kind of magic where merchants don't have control and want to rely on other ways to invest these kind of exceptions to the rule.

While testing I uncover:

Shadow gets cut off I'll investigate further Monday!

@melissaperreault I've pushed a fix for this now. If you could take another look that'd be great!

melissaperreault · 2022-06-14T03:30:47Z

Took another quick look! I am not sure what this is due to but the ratio don't behave the same with the application of the border and shadow to those images now. (Video reference)

allylane · 2022-06-14T18:19:42Z

Took another quick look! I am not sure what this is due to but the ratio don't behave the same with the application of the border and shadow to those images now. (Video reference)

Thanks @melissaperreault. That's fixed, and I've also updated a couple of other spots that media show up to better represent borders and shadows.

assets/section-multicolumn.css

ludoboludo · 2022-06-20T16:02:45Z

assets/base.css

@@ -2832,8 +2834,6 @@ details-disclosure > details {
 .global-media-settings--full-width,
 .global-media-settings--full-width img {
  border-radius: 0;
-  border-left: none;
-  border-right: none;


Is this meant to be removed ? It means that we're going from: current look vs after this borders declarations are removed

Yea, it was intentional, and I think it's nice for imagery to have the borders, but your screenshots highlight that it's not as desirable for text areas. I'll put it back.

ludoboludo · 2022-06-20T16:07:28Z

assets/component-deferred-media.css

-  height: 100%;
-  width: 100%;


Removing those create issues in places where that class is also used like the collage section: video

ok cool. thanks for picking this up.

assets/section-multicolumn.css

ludoboludo · 2022-06-20T18:14:28Z

assets/section-multicolumn.css

 .multicolumn-card > .multicolumn-card__image-wrapper--full-width:not(.multicolumn-card-spacing) {
  border-top-left-radius: calc(var(--text-boxes-radius) - var(--text-boxes-border-width));
  border-top-right-radius: calc(var(--text-boxes-radius) - var(--text-boxes-border-width));
-  overflow: hidden;


I wonder what situation it was needed for before 🤔

Yea, I was wondering the same. I couldn't find anything. cc/ @tyleralsbury @martinamarien

This makes it so that shadow within the content container is overflowing the parent:

Which doesn't look like an ideal situation.

Maybe we could apply the overflow: hidden; only when content containers has at least a 1px border 🤔

ludoboludo · 2022-06-20T18:17:54Z

assets/video-section.css

@@ -1,6 +1,6 @@
 .video-section__media {
  position: relative;
-  padding-bottom: 56.25%;
+  padding-bottom: calc(56.25% - var(--media-border-width) * 2);


Is this change needed ? It seems to be related to the video section only 🤔

ludoboludo · 2022-06-20T18:20:34Z

sections/main-article.liquid

@@ -10,7 +10,7 @@
      {%- when 'featured_image'-%}
        {%- if article.image -%}
          <div class="article-template__hero-container" {{ block.shopify_attributes }}>
-            <div class="article-template__hero-{{ block.settings.image_height }} media"
+            <div class="article-template__hero-{{ block.settings.image_height }} media global-media-settings"


Here I can see how it can be useful to add to articles 👍
Could you add something in the PR description for it as it doesn't currently mentions that there are edits for this section/template.

ludoboludo · 2022-06-20T18:33:57Z

snippets/article-card.liquid

@@ -35,7 +35,7 @@
    >
      <div class="card__inner {% if settings.card_style == 'standard' %} color-{{ settings.card_color_scheme }} gradient{% endif %}{% if article.image and show_image or settings.card_style == 'standard' %} ratio{% endif %}" style="--ratio-percent: {{ 1 | divided_by: ratio | times: 100 }}%;">
        {%- if show_image == true and article.image -%}
-          <div class="article-card__image-wrapper card__media">
+          <div class="article-card__image-wrapper card__media global-media-settings">


Same thing here, if you could add this to your description so that folks reviewing know which part to test.

Here there is more tweaks needed I think depending on the card setup (standard or card).

Also this element would need a border radius applied on it as well otherwise it's not reflecting what the settings are: video

Maybe it we can only tackle multicolumn in the PR to keep it simple and explore the other situation touched here ?

Yea, I may have tackled 1 too many things with this PR

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

…/dawn into Card-image-border-radius

allylane · 2022-06-24T16:09:13Z

@ludoboludo would you mind taking another pass through this with the updates I've made?

ludoboludo · 2022-06-27T18:45:34Z

assets/base.css

 }

 .media--circle {
+  left: calc(-1 * var(--button-size));


Curious to know what this was added for ? 🤔
I don't think we have a --button-size variable declared anywhere else so I don't think this declaration is changing anything.
But let me know if I'm missing something.

ludoboludo · 2022-06-27T18:59:29Z

assets/base.css

@@ -1196,7 +1196,6 @@ summary::-webkit-details-marker {
  display: block;
  background-color: rgba(var(--color-foreground), 0.1);
  position: relative;
-  overflow: hidden;


I tend to get a little worried when removing properties that have impact on many elements. I see that we need to remove this in order for the shadow to come up for multicolumn but maybe instead of removing the property here we could overwrite it in the section-multicolumn.css file with something like:

.multicolumn-card .media { overflow: inherit; }

I feel like it's more "backward compatible".

ludoboludo · 2022-06-27T19:08:40Z

assets/section-multicolumn.css

 .multicolumn-card > .multicolumn-card__image-wrapper--full-width:not(.multicolumn-card-spacing) {
  border-top-left-radius: calc(var(--text-boxes-radius) - var(--text-boxes-border-width));
  border-top-right-radius: calc(var(--text-boxes-radius) - var(--text-boxes-border-width));
-  overflow: hidden;


This makes it so that shadow within the content container is overflowing the parent:

Which doesn't look like an ideal situation.

Maybe we could apply the overflow: hidden; only when content containers has at least a 1px border 🤔

andrewetchen · 2022-07-27T01:20:18Z

Hey team,

The probot-CLA was recently deprecated and replaced with the shopify-cla action.

To successfully use the new shopify-cla status check, this PR will be closed and reopened.

Thanks and sorry for the minor inconvenience. Message me if you have any questions 😄

Please refer to the following for more information:

Alistair Lane added 2 commits June 2, 2022 11:05

Card image style

ec78f7c

Have card images inherit styles from media.

50% border radius for circle images

66d88da

allylane requested review from melissaperreault, martinamarien and tyleralsbury June 2, 2022 15:49

melissaperreault reviewed Jun 3, 2022

View reviewed changes

Make borders work for circles

e2d0cbc

allylane requested a review from andrewetchen June 13, 2022 19:24

Media updates

57649ae

ludoboludo reviewed Jun 20, 2022

View reviewed changes

Alistair Lane and others added 5 commits June 24, 2022 11:15

Update assets/section-multicolumn.css

fd2f8d1

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

Reverting some changes

e8da4a6

Merge branch 'Card-image-border-radius' of https://github.com/Shopify…

73cb59b

…/dawn into Card-image-border-radius

Revert

2fbd2f4

Simplifying changes

dff7148

ludoboludo reviewed Jun 27, 2022

View reviewed changes

KaichenWang added the By: Team label Jul 18, 2022

andrewetchen closed this Jul 27, 2022

andrewetchen reopened this Jul 27, 2022

martinamarien removed their request for review March 6, 2024 15:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Card image border radius #1764

Card image border radius #1764

allylane commented Jun 2, 2022 •

edited

Loading

melissaperreault left a comment •

edited

Loading

allylane commented Jun 7, 2022

allylane commented Jun 13, 2022

melissaperreault commented Jun 14, 2022 •

edited

Loading

allylane commented Jun 14, 2022

ludoboludo Jun 20, 2022

allylane Jun 24, 2022

ludoboludo Jun 20, 2022

allylane Jun 24, 2022

ludoboludo Jun 20, 2022

allylane Jun 24, 2022

ludoboludo Jun 27, 2022

ludoboludo Jun 20, 2022

ludoboludo Jun 20, 2022

ludoboludo Jun 20, 2022

allylane Jun 24, 2022

allylane commented Jun 24, 2022

ludoboludo Jun 27, 2022

ludoboludo Jun 27, 2022

ludoboludo Jun 27, 2022

andrewetchen commented Jul 27, 2022

Card image border radius #1764

Are you sure you want to change the base?

Card image border radius #1764

Conversation

allylane commented Jun 2, 2022 • edited Loading

melissaperreault left a comment • edited Loading

Choose a reason for hiding this comment

allylane commented Jun 7, 2022

allylane commented Jun 13, 2022

melissaperreault commented Jun 14, 2022 • edited Loading

allylane commented Jun 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allylane commented Jun 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewetchen commented Jul 27, 2022

allylane commented Jun 2, 2022 •

edited

Loading

melissaperreault left a comment •

edited

Loading

melissaperreault commented Jun 14, 2022 •

edited

Loading