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

add override for mobile devices to show smaller swatches #3220

Closed
wants to merge 2 commits into from

Conversation

mkoenig-shopify
Copy link
Contributor

PR Summary:

Really small PR to make the swatches a bit smaller on mobile. This will optimize the way they look/feel when selecting variants.

Why are these changes introduced?

We got feedback that the swatches were too large on mobile at 4.4rem - so we reduced them to 3.6rem

What approach did you take?

A media class in CSS

Visual impact on existing themes

Before:
Screenshot 2024-01-24 at 1 30 53 PM

After:
Screenshot 2024-01-24 at 1 31 05 PM

Testing steps/scenarios

Demo links

Checklist

Copy link
Member

@alisterdev alisterdev left a comment

Choose a reason for hiding this comment

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

Since this goes against a11y guidelines, do we have a longer term plan to address this?

@@ -10,6 +10,12 @@
forced-color-adjust: none;
}

@media screen and (max-width: 750px) {
Copy link
Member

@alisterdev alisterdev Jan 24, 2024

Choose a reason for hiding this comment

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

You don't need the @media query here, we already override the size for the larger screens here:

@media screen and (min-width: 750px) {
.product-form__input--swatch .swatch-input__input + .swatch-input__label {
--swatch-input--size: 2.8rem;
}

@@ -1,15 +1,27 @@
/* swatch-input lives in its own file for reusability of the swatch in other areas than the product form context */

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

align-items: center

}

Copy link
Contributor

Choose a reason for hiding this comment

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

Here are a couple funky things I noticed:

Desktop looks a bit off now

Here is how it looks when receiving focus

Copy link
Contributor

Choose a reason for hiding this comment

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

The focus rings are going to be a really tricky part of this. I don't suggest trying to include this in the next release, personally.

@tyleralsbury
Copy link
Contributor

@mkoenig-shopify Is this still something we want to try to pursue?

@mkoenig-shopify
Copy link
Contributor Author

@tyleralsbury it is yes but we got pulled into some P0s, planning to come back to this in march

@mkoenig-shopify
Copy link
Contributor Author

closing in favor of #3348 (review)

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.

4 participants