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

Switch the filter swatches to use the swatch api #3303

Merged
merged 2 commits into from
Feb 28, 2024
Merged

Switch the filter swatches to use the swatch api #3303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fa246fa
Switch swatch filters to use swatch API
jas7457 Feb 27, 2024
2ad1abf
Rename to facets__label
jas7457 Feb 28, 2024
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
28 changes: 12 additions & 16 deletions assets/component-facets.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,8 +300,10 @@
}

.facets-layout-grid {
--swatch-input--size: 2.4rem;

display: grid;
grid-template-columns: repeat(3, 1fr);
grid-template-columns: repeat(3, minmax(0, 1fr));
text-align: center;
padding: 2rem 2.4rem;
gap: 3rem 1rem;
Expand All @@ -326,7 +328,7 @@
align-items: flex-start;
}

.facets-layout-grid .visual-display-parent {
.facets-layout-grid .facets__label {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is overwriting the styles being applied by the .facet-checkbox declaration. That's fine to overwrite them... the weird part is that .facet-checkbox is setting the display, flex-direction, etc to begin with. Two things to change that could fix the alignment of list items with a checkbox:

  1. Change .facet-checkbox on line 345 to .facets__label. This is in line with a number of other changes you made that removes the focus on facet-chackbox to the labels.
  2. Add align-items: center to that declaration.

This would fix a misalignment that's most visible in the mobile's drawer view

You can also see it in the vertical layout; this is what it looks like with the fix applied

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I talked with @NathanPJF about this. If the text wraps, then the checkbox (but not the checkmark) would get centered. We can fix the checkmark as well, but in either case, we decided having the checkbox centered looked worse, so we won't make this change. I can do the rename, but not the alignment.

image

display: flex;
flex-direction: column;
gap: 0.8rem;
Expand All @@ -340,7 +342,7 @@
cursor: pointer;
}

.facet-checkbox {
.facets__label {
padding: 1rem 2rem 1rem 0;
flex-grow: 1;
position: relative;
Expand All @@ -361,7 +363,13 @@
}
}

.facet-checkbox input[type='checkbox'] {
/* Disabled state */
.facets__label.disabled {
opacity: 0.4;
pointer-events: none;
}

.facets-layout-list input[type='checkbox'] {
position: absolute;
opacity: 1;
width: 1.6rem;
Expand All @@ -384,12 +392,6 @@
opacity: 0;
}

.facets__visual-display-wrapper {
display: flex;
justify-content: center;
flex-shrink: 0;
}

.no-js .facet-checkbox input[type='checkbox'] {
z-index: 0;
}
Expand Down Expand Up @@ -423,12 +425,6 @@
}
}

.facet-checkbox--disabled,
.mobile-facets__label--disabled {
opacity: 0.4;
pointer-events: none;
}

.facets__price {
display: flex;
padding: 2rem;
Expand Down
2 changes: 2 additions & 0 deletions assets/component-product-variant-picker.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,8 @@ variant-selects {
}

.product-form__input--swatch .swatch-input__input + .swatch-input__label {
--swatch-input--size: 4.4rem;

margin: 0.7rem 1.2rem 0.2rem 0;
}

Expand Down
10 changes: 8 additions & 2 deletions assets/component-swatch-input.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
/* swatch-input lives in its own file for reusability of the swatch in other areas than the product form context */
.swatch-input__input + .swatch-input__label {
--swatch-input--size: 4.4rem;
--swatch-input--border-radius: 50%;

display: inline-block;
max-width: 100%;
border-radius: var(--swatch-input--border-radius);
cursor: pointer;
outline-offset: 0.2rem;
Expand All @@ -14,7 +14,8 @@
--swatch-input--border-radius: 0.2rem;
}

.swatch-input__input + .swatch-input__label:hover {
.swatch-input__input + .swatch-input__label:hover,
.swatch-input__input:hover + .swatch-input__label {
outline: 0.2rem solid rgba(var(--color-foreground), 0.4);
}

Expand All @@ -28,6 +29,11 @@
outline: none;
}

/* Actually disabled */
.swatch-input__input:disabled + .swatch-input__label {
pointer-events: none;
}

/* Focus visible */
.swatch-input__input:focus-visible + .swatch-input__label {
box-shadow: 0 0 0 0.5rem rgb(var(--color-background)), 0 0 0 0.7rem rgba(var(--color-foreground), 0.55);
Expand Down
2 changes: 2 additions & 0 deletions assets/component-swatch.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@

display: block;
width: var(--swatch--size);
max-width: 100%;
aspect-ratio: 1 / 1;
background: var(--swatch--background);
background-position: var(--swatch-focal-point, initial);
background-size: cover;
background-origin: border-box;
border: 0.1rem solid rgba(var(--color-foreground), 0.45);
Expand Down
90 changes: 0 additions & 90 deletions assets/component-visual-display.css
View file
Open in desktop

This file was deleted.

5 changes: 5 additions & 0 deletions assets/global.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1015,6 +1015,11 @@ class VariantSelects extends HTMLElement {
selectedDropdownSwatchValue.style.setProperty('--swatch--background', 'unset');
selectedDropdownSwatchValue.classList.add('swatch--unavailable');
}

selectedDropdownSwatchValue.style.setProperty(
'--swatch-focal-point',
target.selectedOptions[0].dataset.optionSwatchFocalPoint || 'unset'
);
Comment on lines +1019 to +1022
Copy link
Contributor

Choose a reason for hiding this comment

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

This means it will add this even though there are no images being used 🤔
We could run it only when necessary but I guess it doesn't matter as the unset does no harm and prevent added logic 👍

} else if (tagName === 'INPUT' && target.type === 'radio') {
const selectedSwatchValue = this.querySelector(`[data-selected-swatch-value="${name}"]`);
if (selectedSwatchValue) selectedSwatchValue.innerHTML = value;
Expand Down
Loading
Loading