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

Remove shadows from variant images in the quick add modal #3449

Merged
merged 1 commit into from
May 3, 2024
+4 −0
Merged

Remove shadows from variant images in the quick add modal #3449

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions assets/quick-order-list.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ quick-order-list .quantity__button {
z-index: 1;
}

.variant-item__image-container.global-media-settings::after {
content: none;
}

Comment on lines +39 to +42
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like the border could also be removed from the quick add - bulk and quick order list, specifically for the row images.
Maybe we could set a max value actually on them 🤷 using min so it still have a border but prevents it from being huge and actually hiding the image:

min(var(--media-border-width), 5px)
Here is how it looks with 14px border thickness

And here with 24px thickness

Copy link
Contributor

Choose a reason for hiding this comment

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

I dont know about setting a max. I think that goes against our principles 🤔 We dont really do that anywhere else and let the merchants go as far as theyd like

I think we either remove it (and in that case maybe all of the other settings too?) or keep it as is. It feels a bit odd to pick and choose which global settings apply and which dont

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I don't know if we need to be that picky. Removing the shadows from the quick add modal was an obvious must, they were always either rendering broken or not at all.

Even removing them from quick order list feels a little heavy-handed to me, like what's really wrong with a subtle 2px shadow. Borders are in the same boat—yes you can do weird things if you try to break it, but if you have a light theme and light product images, a dark 1px border might be really important for your images to render cleanly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that's why I would see it as our responsibility to put in guard rails where possible. Expecting the merchants to set a some global settings to a value and go check everywhere in the theme what could be impacted isn't the way to go imo.

If you have 10px thickness for your border, it's not even pushing it to the max at 24px but it's not looking good as a user. The issue being it's applied on images that are bigger where it's not a problem but those instances where it's tiny are worst (quick add modal bulk).

Once they see it they can always reduce their border thickness and mitigate it 🤷 So it's not the worst.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the concerns, we made a few exceptions to this rule and accepted that border may not look good for all components and that it's a pretty edgy style to apply to your online store.

We are okay for setting not looking good in all use cases for theme settings, we have this issue everywhere and we rely on merchants to make the best decision for their brand, but the shadow cutting off, I am ok to remove it on the poster product image if it's a pain to adjust.

@media screen and (min-width: 990px) {
.quick-order-list__total {
position: sticky;
Expand Down
Loading