Add visual representation for filters #3045
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -272,11 +272,42 @@ | |
| padding: 0.5rem 2rem; | ||
| } | ||
|
|
||
| .facets-layout-grid { | ||
| display: grid; | ||
| grid-template-columns: repeat(3, 1fr); | ||
| text-align: center; | ||
| padding: 2rem 2.4rem; | ||
| gap: 3rem 1rem; | ||
| } | ||
|
|
||
| .facets-layout-grid.facets__list--vertical { | ||
| padding: 1rem 0; | ||
| } | ||
|
|
||
| .facets__item { | ||
|
There was a problem hiding this comment. Have we thought about what happens if one of the colors match the background color? If the background is red for instance, and the colors have one color that is red. There was a problem hiding this comment. I see we have the special case for white, but we should ensure it works for whatever the background color is 👀 There was a problem hiding this comment. Hmm we don't have any special casing for white, it should work the same no matter the background color. We put a border around it like we can see in your screenshot, and the hover/focus/active states will make it stand out as well. Do you have any suggestions for any changes? I'd think the border around the swatch/thumbnail is all we need. https://screenshot.click/12-58-s8ep1-tb2lw.mp4 There was a problem hiding this comment. What I mean is that the border around the white is dotted while the others arent. That works when the background is white, but not when you change it to a different color 🤔 @melissaperreault thoughts? There was a problem hiding this comment. The dotted border is what shows for the empty state. In your video, you must not have a valid visual representation for "Greens" or "White", it isn't special cased for white. For example, if you look at the app you can see you don't have those two properly set.
There was a problem hiding this comment. Ah I see that makes sense 👍 There was a problem hiding this comment. @al-mcquiston and I am aligned it was the right UI visual to illustrate a missing value. Thanks for double checking @sofiamatulis ! |
||
| display: flex; | ||
| align-items: center; | ||
| } | ||
|
|
||
| /* Hover/focus state */ | ||
| .facets-layout-list .facets__label:hover .facet-checkbox__text, | ||
| .facets-layout-list input:focus ~ .facet-checkbox__text { | ||
| text-decoration: underline; | ||
| } | ||
|
Comment on lines
+293
to
+296
There was a problem hiding this comment. (remark) Although this is fine IMO, I just wanted to point out that before, the hover state on the I do think this is a good change thought. There was a problem hiding this comment. Yeah the non-mobile didn't have an underline before either. Al wanted to add it, he didn't specify if it should only be applied to desktop, but I don't think we should have different styles between the two (or else we'll continue to have spaghetti code in the future) |
||
|
|
||
| .facets-layout-grid > * { | ||
| align-items: flex-start; | ||
| } | ||
|
|
||
| .facets-layout-grid .visual-display-parent { | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 0.8rem; | ||
| padding: 0; | ||
| height: 100%; | ||
| font-size: 1.3rem; | ||
| } | ||
|
|
||
| .facets__item label, | ||
|
There was a problem hiding this comment. When showing multiple colors, it's showing as white. Is that expected? https://screenshot.click/12-00-qb4vh-ppeei.mp4 There was a problem hiding this comment. That's because when we set up the filter, we set the override to Image (which doesn't look like you're using).
Instead, you can click the pencil/edit icon and change Swatch override to Colors and save. |
||
| .facets__item input[type='checkbox'] { | ||
| cursor: pointer; | ||
|
|
@@ -291,6 +322,18 @@ | |
| word-break: break-word; | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. I wonder if we have a different threshold for colors in terms of show more? For text, 10 makes sense because the list is long, but for color I feel like we could have more rows before showing There was a problem hiding this comment. I think that's a good idea. I'm thinking 24 since it's divisible by 3 and feels like it has a good balance between not TOO many, but not too few. I'd only do this for things that have a "visual display", it wouldn't be attached to "color" in any way. There was a problem hiding this comment. I think that makes sense 👍 Ill let @melissaperreault confirm There was a problem hiding this comment.
There was a problem hiding this comment. Changed to use 15 for swatch (5 rows), 12 for thumbnail (4 rows) |
||
| /* Hover, active, and focus states */ | ||
| :is(.facets__label:hover, .facets__label.active, .facets__label:has(:focus-visible)) { | ||
| color: rgba(var(--color-foreground), 1); | ||
| } | ||
|
|
||
| /* Focus states for older browsers */ | ||
| @supports not selector(:has(a, b)) { | ||
| .facets__label:focus-within { | ||
| color: rgba(var(--color-foreground), 1); | ||
| } | ||
| } | ||
|
Comment on lines
+331
to
+335
There was a problem hiding this comment. (question) Couldn't we just put the rule as one of the rules in the There was a problem hiding this comment. No, because we don't want this to apply by default. If we did this, when you click on a filter, it has the 
|
||
|
|
||
| .facet-checkbox input[type='checkbox'] { | ||
| position: absolute; | ||
| opacity: 1; | ||
|
|
@@ -303,6 +346,23 @@ | |
| -webkit-appearance: none; | ||
| } | ||
|
|
||
| .facets-layout-grid input[type='checkbox'] { | ||
| position: absolute; | ||
| top: 0; | ||
| left: 0; | ||
| right: 0; | ||
| bottom: 0; | ||
| width: 100%; | ||
| height: 100%; | ||
| opacity: 0; | ||
| } | ||
|
|
||
| .facets__visual-display-wrapper { | ||
| display: flex; | ||
| justify-content: center; | ||
| flex-shrink: 0; | ||
| } | ||
|
|
||
| .no-js .facet-checkbox input[type='checkbox'] { | ||
| z-index: 0; | ||
| } | ||
|
|
@@ -336,8 +396,10 @@ | |
| } | ||
| } | ||
|
|
||
| .facet-checkbox--disabled, | ||
| .mobile-facets__label--disabled { | ||
| opacity: 0.4; | ||
| pointer-events: none; | ||
| } | ||
|
|
||
| .facets__price { | ||
|
|
@@ -744,20 +806,20 @@ details.menu-opening .mobile-facets__close svg { | |
| } | ||
|
There was a problem hiding this comment. This is happening on mobile: https://screenshot.click/12-04-wo88b-bk2tr.mp4 There was a problem hiding this comment. Good call out. I had added some styles that are no longer necessary. Instead, I got rid of those styles and set the width to 100% (to match the grid cell) in the case that we're inside of the Note: the reason this is necessary is our "default" should be 9rem (if this is used outside of filters), but that doesn't actually fill the width for our desktop filters. If we don't force it to 100% there, we will get extra spacing on the left/right, which we don't want, so I can just force it to 100% width. 
|
||
|
|
||
| .mobile-facets__highlight { | ||
| position: absolute; | ||
| top: 0px; | ||
| left: 0px; | ||
| right: 0px; | ||
| bottom: 0px; | ||
| display: block; | ||
| background-color: rgba(var(--color-foreground), 0.04); | ||
| opacity: 0; | ||
| visibility: hidden; | ||
| } | ||
|
|
||
| .mobile-facets__checkbox:checked + .mobile-facets__highlight { | ||
| opacity: 1; | ||
| visibility: visible; | ||
andy-schneidr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
|
There was a problem hiding this comment. Is there a reason why certain colors show the opacity different when unavailable while some have the There was a problem hiding this comment. They should all have the There was a problem hiding this comment. You're saying the contrast is not enough where at times you cant see the There was a problem hiding this comment. We use the current foreground color at 40% opacity, along with setting the opacity of the image itself to 40%. Perhaps we could boost the opacity to make it more apparent, but the opacity itself also signifies it's disabled. Any suggestions? There was a problem hiding this comment. I was noticing this too and think it's something we should fix. Is there a way we can just not apply the decreased opacity to the /? There was a problem hiding this comment. We updated the |
||
| .mobile-facets__summary { | ||
|
|
@@ -854,10 +916,6 @@ input.mobile-facets__checkbox { | |
| display: block; | ||
| } | ||
|
|
||
| .mobile-facets__footer { | ||
| border-top: 0.1rem solid rgba(var(--color-foreground), 0.08); | ||
| padding: 2rem; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| .visual-display { | ||
| --visual-display__size: min(2.4rem, 100%); | ||
| position: relative; | ||
| width: var(--visual-display__size); | ||
| max-width: 100%; | ||
| border: 0.1rem solid rgba(var(--color-foreground), 0.2); | ||
| aspect-ratio: 1/1; | ||
| } | ||
|
|
||
| .visual-display.empty { | ||
| border-style: dashed; | ||
| } | ||
|
|
||
| .visual-display--presentation-swatch { | ||
| --visual-display__size: min(2.4rem, 100%); | ||
|
|
||
| border-radius: 100%; | ||
| overflow: hidden; | ||
| } | ||
|
|
||
| .visual-display-parent .visual-display--presentation-swatch { | ||
| outline-offset: 0.2rem; | ||
| } | ||
|
|
||
| /* Hover, active, and focus states */ | ||
| :is( | ||
| .visual-display-parent:hover .visual-display--presentation-swatch, | ||
| .visual-display-parent.active .visual-display--presentation-swatch, | ||
| .visual-display-parent:has(:focus-visible) .visual-display--presentation-swatch | ||
| ) { | ||
| outline-style: solid; | ||
| } | ||
|
|
||
| /* Active state */ | ||
| .visual-display-parent.active .visual-display--presentation-swatch { | ||
| outline-width: 0.1rem; | ||
| outline-color: rgb(var(--color-foreground), 1); | ||
| } | ||
|
|
||
| /* Hover state */ | ||
| .visual-display-parent:hover .visual-display--presentation-swatch { | ||
| outline-width: 0.2rem; | ||
| outline-color: rgb(var(--color-foreground), 0.4); | ||
| } | ||
|
|
||
| /* Focus state */ | ||
| .visual-display-parent:has(:focus-visible) .visual-display--presentation-swatch { | ||
| outline-width: 0.2rem; | ||
| outline-color: rgb(var(--color-foreground), 0.4); | ||
| box-shadow: 0 0 0 0.6rem rgb(var(--color-background)), 0 0 0 0.8rem rgba(var(--color-foreground), 0.5), | ||
| 0 0 1.2rem 0.4rem rgba(var(--color-foreground), 0.3); | ||
| } | ||
|
|
||
| /* Focus state for older browsers */ | ||
| @supports not selector(:has(a, b)) { | ||
|
There was a problem hiding this comment. What is There was a problem hiding this comment. Essentially, they don't matter. This is more just testing "does has work", not "does has with certain params/attributes work". Taken from mdn There was a problem hiding this comment. Thanks for the clarification. Im not sure if we need this here 🤔 Is there a reason why we are using In Dawn, we have used https://github.com/search?q=repo%3AShopify%2Fdawn+%3Afocus&type=code There was a problem hiding this comment. So this is a pretty unique scenario, but I essentially want to know if anything inside of Essentially I have this class |
||
| .visual-display-parent:focus-within .visual-display--presentation-swatch { | ||
| outline-offset: 0.2rem; | ||
| outline: 0.2rem solid rgb(var(--color-foreground), 0.4); | ||
| box-shadow: 0 0 0 0.6rem rgb(var(--color-background)), 0 0 0 0.8rem rgba(var(--color-foreground), 0.5), | ||
| 0 0 1.2rem 0.4rem rgba(var(--color-foreground), 0.3); | ||
| } | ||
| } | ||
|
|
||
| .visual-display-parent.disabled { | ||
| opacity: 0.4; | ||
| pointer-events: none; | ||
| } | ||
|
|
||
| /* Used to display the disabled dash */ | ||
| .visual-display-parent.disabled .visual-display::after { | ||
| display: block; | ||
| content: ''; | ||
|
|
||
| /* 1.414 is not a magic number, it's the square root of 2, or the length of the diagonal */ | ||
|
|
||
| width: calc(var(--visual-display__size) * 1.414); | ||
| border-bottom: 0.1rem solid rgb(var(--color-background-contrast)); | ||
| transform: rotate(-45deg); | ||
| transform-origin: left; | ||
| } | ||
|
|
||
| .visual-display .visual-display__child { | ||
| display: block; | ||
| height: 100%; | ||
| width: 100%; | ||
| } | ||
|
|
||
| .visual-display--presentation-swatch .visual-display__image { | ||
| object-fit: cover; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new filters dont show on the search template: https://screenshot.click/12-43-0fizq-qn1ea.mp4
Is that expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're looking at the wrong "Color" filter, remember you had two set up? Since you've searched for "bag", that doesn't match the product you attached the visual filter to, so it won't show up.
You could search for "*" instead and you should see it.