Refactor quick-order-list.css

[andrewetchen]

PR Summary

This PR refactors the quick-order-list.css file by removing any duplicate or redundant CSS.

Why are these changes introduced?

Fixes #2956

What approach did you take?

• I went through line-by-line and removed any duplicate or redundant code.
• I removed a couple of rules that had incorrect class names, missing ;'s, or had spelling errors.
• I repositioned some rules and declarations.
• I ensured the removal of any code didn't break anything.
• I made sure to compare against the component-cart-items.css file since a lot of the CSS was taken from it.

Other considerations

• The quick-order-list.css file could probably benefit from further refactoring (e.g., organizing the same @media blocks into one vs. having multiple blocks) but I see this as a nice-to-have and something that doesn't need to be scoped into this PR.
• We can also create a shared stylesheet (e.g., table.css) for similarities between quick-order-list.css and component-cart-items.css. This would also require additional changes to the markup.
• Edit (October 4 2023): We're not applying the max-height and overflow-y to the quick order list container and will tackle this separately
  • Refactor quick-order-list.css #3006 (comment)
• Considerations from @eugenekasimov
  • When you add a variant to the cart via the quick-order-list a horizontal scrolling bar appears for a loading period of time.
  • Edit (October 4 2023): Sofia and I have decided to not include the max-height and overflow-y to the quick order list container so the horizontal scrolling is not an issue in this PR
    • Refactor quick-order-list.css #3006 (comment)

Note: This PR includes Eugene's fix from #3003

Testing steps/scenarios

Use the Dawn main editor link below to help with comparing these changes.

• Ensure the quick order list section works as expected
• Test all screen sizes
• Test on different web browsers

Demo links

• Editor with refactored quick-order-list.css (This PR)
• Editor of Dawn main

Checklist

• Added PR summary for release notes
• Requested review from UX (Only for changes that are affecting the experience or perceivable visual details)
• Created a ticket for the help.shopify.com documentation team about updates to theme settings. (Internal-only task)
• Followed theme code principles
• Linted with Theme Check
• Tested on mobile
• Tested on multiple browsers
• Tested for accessibility

[eugenekasimov]

Looks good so far. I will keep reviewing it tmr.

[eugenekasimov]

We need to rethink about it because it causes an issue with scrolling 👇

https://screenshot.click/25-36-u81os-rw42j.mp4

[andrewetchen]

Thanks Eugene, it was suggested to add this back here but for another class: #3006 (comment)

I'm wondering if we should just remove it since it wasn't ever being used? CC: @sofiamatulis

[sofiamatulis]

For some context, this was accidentally not added in the release because of the class change names. But it was something @edmund-teh had requested for and the scrolling was something we knew about and aligned that was ok

So let's get Ed's opinion. But here are the options

1. Add the scrolling that was supposed to be there, but accidentally didnt because of the class name changes. This means the scroll will show. The max height was important so the section itself is not long. This means this could become a very long section and the bottom footer doesnt show up

2. Keep it as is, but that means there is no max height. Since merchants can add many sections, that could be an issue as the page could get very long

Video: https://screenshot.click/25-52-czeo6-n3am3.mp4

[andrewetchen]

Thanks for the context, Sofia. I'm aligned with Option 1

The max height was important so the section itself is not long. This means this could become a very long section and the bottom footer doesn't show up

That's the first thing I noticed as a good thing when I added this here: 00557a5

I don't mind the scrollbars showing like in Eugene's video, especially if you and @edmund-teh aligned on this previously.

[eugenekasimov]

ah, I recall this actually. Let me drop a short video here why I was initially against adding max-height and overflow-y.
https://screenshot.click/25-02-hwmw2-3h4x2.mp4

Of course it's up to @edmund-teh. I think if we decide to go with max-height and scrolling bar then we need to add paddings on the right for both the table and total bar and somehow make it work equally for users who use touchpad and the mouse.

[edmund-teh]

Hey everyone! Thanks for being thorough and discussing this.

I'm aligned with adding the max-height and scrolling (Option 1).

I do also see the value in @eugenekasimov's style tweaks that would account for a sidebar to the right of the totals. But if we were to implement these changes, we'd also have to adjust this for the footer, as well:
https://screenshot.click/Screenshot_2023-09-25_at_4.54.43_PM.png

These style tweaks would be accounting for whatever default browser scroll bar the buyer is using, so that may have a lot of variation in itself, right? I think we would have to decide if this is worth the effort (from a Product decision, perspective, cc: @nicklepine).

This is also something we'd probably implement for Quick order form, too? So, maybe we would need @scottgmeadows's input, as well?

cc: @sofiamatulis @andrewetchen

[andrewetchen]

Thanks @edmund-teh

One thing I wanted to point out (might help us decide) is that we already have a similar behaviour with a visible scrollbar but in this context, it's always visible regardless of your System Settings > Appearance > Show scroll bars setting -- whether it's Automatically based on mouse or trackpad, When scrolling, or Always:

Set the Cart type to Drawer and add multiple items to your cart, you can see the vertical scrollbar. This happens in Chrome and Safari but not in FireFox and this was something the team was okay with:

Here are 2 videos showing how the different Show scroll bars settings affect the cart drawer and quick order list:

show-scrollbars-setting-cart-drawer.mp4

show-scrollbars-setting-quick-order-list.mp4

[edmund-teh]

One thing I wanted to point out (might help us decide) is that we already have a similar behaviour with a visible scrollbar but in this context, it's always visible regardless of your System Settings > Appearance > Show scroll bars setting -- whether it's Automatically based on mouse or trackpad, When scrolling, or Always:

Good point @andrewetchen, if it's already something we're experiencing with other interactions in Dawn, we should probably keep things consistent.

[eugenekasimov]

I really like an approach we use for the cart-drawer when we see a scrolling bar all the time no matter what settings/devices are set/used. 👍

[andrewetchen]

Thanks everyone. This thread has a lot of great context!

Good point @andrewetchen, if it's already something we're experiencing with other interactions in Dawn, we should probably keep things consistent.

@edmund-teh and I had a chat about this. We're good to move forward with the visible scrollbars (with Show scroll bars setting: Automatically based on mouse or trackpad or Always).

cc: @sofiamatulis @eugenekasimov

[eugenekasimov]

Great job, Andrew!

I added my concern to the description for this PR.

[sofiamatulis]

I am testing with all the settings to ensure there are no regressions. I have noticed one thing

Ill do one final pass. Here are the scenarios:

• no JS
• mobile
• tablet
• desktop
• no sku/sku
• no image/image
• error states
• with qty rules/vol pricing

[sofiamatulis]

Noticed a few more things - i can do one final pass once theyre fixed 👌

[sofiamatulis]

I noticed this was introduced on this PR https://screenshot.click/29-56-4zuwn-hkbg9.mp4

the popover is showing behind

[andrewetchen]

What's strange is that this is coming from this: 7e2238a

#3006 (comment)

quick-order-list-container.mp4

[sofiamatulis]

Yes, I think it's due to the overflow-y. https://screenshot.click/03-43-z9vco-k9eqv.mp4

Have you seen if there is a way around it?

We should test it with a long list of variants that has qty rules/vol pricing

[andrewetchen]

That's the first thing I noticed while testing this. When I remove the overflow-y from the following it works as expected:

.quick-order-list__container {
  padding-bottom: 4rem;
  max-height: 90rem;
  overflow-y: auto;
}

Have you seen if there is a way around it?

I'll try to tweak things 👍🏼

[andrewetchen]

Here's a solution to fix this -- let me know what you think: 7d26147

[sofiamatulis]

This is happening now when you start scrolling: https://screenshot.click/04-28-ptcgs-w8vk4.mp4

(comparing this branch to main in the video)

[andrewetchen]

This is happening now when you start scrolling: https://screenshot.click/04-28-ptcgs-w8vk4.mp4

(comparing this branch to main in the video)

Hey @sofiamatulis, as we discussed on Slack, the current issue seems to be a side-effect of applying the following CSS:

• Refactor quick-order-list.css #3006 (comment)
• Refactor quick-order-list.css #3006 (comment)

.quick-order-list__container {
  max-height: 90rem;
  overflow-y: auto;
}

When this CSS is applied, we're encountering one of two issues:

• The popover appearing behind (screenshot)
• The popover scrolling (screenshot)

Given the complexity of these issues and since this CSS wasn't added to the latest release, I propose we descope this from the current refactoring PR. Instead, we can address the max-height addition to the container in a separate, follow-up PR. This way, we can focus on resolving these issues without delaying the current refactoring work.

Let me know your thoughts on this approach.

[sofiamatulis]

This is happening now when you start scrolling: https://screenshot.click/04-28-ptcgs-w8vk4.mp4
(comparing this branch to main in the video)

Hey @sofiamatulis, as we discussed on Slack, the current issue seems to be a side-effect of applying the following CSS:

• Refactor quick-order-list.css #3006 (comment)
• Refactor quick-order-list.css #3006 (comment)

.quick-order-list__container {
  max-height: 90rem;
  overflow-y: auto;
}

When this CSS is applied, we're encountering one of two issues:

• The popover appearing behind (screenshot)
• The popover scrolling (screenshot)

Given the complexity of these issues and since this CSS wasn't added to the latest release, I propose we descope this from the current refactoring PR. Instead, we can address the max-height addition to the container in a separate, follow-up PR. This way, we can focus on resolving these issues without delaying the current refactoring work.

Let me know your thoughts on this approach.

I am aligned :) Let's revert the removal of the position: relative and the addition of the max height and scroll and we can tackle this on a separate PR and get UX eyes on it too

[sofiamatulis]

Looks good 🎉 thanks for tackling this