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.

Sign up for GitHub

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

Jump to bottom

Card image border radius #1764

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
+22 −8
Open

Card image border radius #1764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ec78f7c
Card image style
Jun 2, 2022
66d88da
50% border radius for circle images
Jun 2, 2022
e2d0cbc
Make borders work for circles
Jun 13, 2022
57649ae
Media updates
Jun 14, 2022
fd2f8d1
Update assets/section-multicolumn.css
Jun 24, 2022
e8da4a6
Reverting some changes
Jun 24, 2022
73cb59b
Merge branch 'Card-image-border-radius' of https://github.com/Shopify…
Jun 24, 2022
2fbd2f4
Revert
Jun 24, 2022
dff7148
Simplifying changes
Jun 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions assets/base.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1196,7 +1196,6 @@ summary::-webkit-details-marker {
display: block;
background-color: rgba(var(--color-foreground), 0.1);
position: relative;
overflow: hidden;
Copy link
Contributor

Choose a reason for hiding this comment

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

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".

}

.media--transparent {
Expand All @@ -1221,26 +1220,28 @@ summary::-webkit-details-marker {
}

.media--square {
padding-bottom: 100%;
padding-bottom: calc(100% - var(--media-border-width) * 2);
}

.media--portrait {
padding-bottom: 125%;
padding-bottom: calc(125% - var(--media-border-width) * 2);
}

.media--landscape {
padding-bottom: 66.6%;
padding-bottom: calc(66.6% - var(--media-border-width) * 2);
}

.media--cropped {
padding-bottom: 56%;
padding-bottom: calc(56% - var(--media-border-width) * 2);

}

.media--16-9 {
padding-bottom: 56.25%;
padding-bottom: calc(56.25% - var(--media-border-width) * 2);
}

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

Choose a reason for hiding this comment

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

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.

padding-bottom: 100%;
border-radius: 50%;
}
Expand All @@ -1251,7 +1252,7 @@ summary::-webkit-details-marker {

@media screen and (min-width: 990px) {
.media--cropped {
padding-bottom: 63%;
padding-bottom: calc(63% - var(--media-border-width) * 2);
}
}

Expand Down
15 changes: 14 additions & 1 deletion assets/section-multicolumn.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,10 +200,23 @@
box-sizing: border-box;
}

.multicolumn-card .multicolumn-card__image {
border: var(--media-border-width) solid rgba(var(--color-foreground), var(--media-border-opacity));
border-radius: var(--media-radius);
box-shadow: var(--media-shadow-horizontal-offset)
var(--media-shadow-vertical-offset)
var(--media-shadow-blur-radius)
rgba(var(--color-shadow), var(--media-shadow-opacity));
}
ludoboludo marked this conversation as resolved.
Show resolved Hide resolved

.multicolumn-card .media--circle,
.media--circle .multicolumn-card__image {
border-radius: 50%;
}

.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;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder what situation it was needed for before 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 🤔

}

.multicolumn.background-none .multicolumn-card {
Expand Down