-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[Quick order form] Add sticky header #3132
base: quick-order-form
Are you sure you want to change the base?
Conversation
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.
Looking at the demo store first then will look at the code :)
Did the first pass for design and performance. I noticed one more thing apart the one Sofia mentioned above. The current sticky element covers the upcoming product title and it has a bit of a wrong effect in my opinion. Here is a short video with explanation. https://screenshot.click/29-11-9z70t-9hpnm.mp4 |
Thank you Andrew for the well detailed PR! 🙌 🙌
I agree, but I think this can be easily solvable with some transitions and momentum. I can see whether we fade out/slide the sticky header once we hit the last variant, and fade in/slide. Observations
|
e4f532e
to
38b09d8
Compare
position: sticky; | ||
top: 0; | ||
z-index: 3; /* Prevent volume pricing popover overlap */ | ||
background-color: rgb(var(--color-background)); |
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.
Hey Andrew. It seems you permanently change the background-color
of the thead
. The background-color
of the thead
is already declared here and I think we want to keep this color until the sticky header is triggered. When it's triggered we need to change it to the color you declare here, then as we scroll up back we need to return the original color.
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.
I see your point. I can add a class to the thead
when the sticky header is activated and remove it when scrolling back up. However, due to the way we trigger the sticky header change, we might experience a momentary transparent effect before it updates (video).
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.
To your point earlier (here), this could be a good reason to adjust when the sticky header is activated.
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.
@melissaperreault just sharing / for context. I'll be exploring activating the sticky header sooner.
@@ -2326,7 +2326,7 @@ product-info .loading__spinner:not(.hidden) ~ *, | |||
|
|||
/* section-header */ | |||
.section-header.shopify-section-group-header-group { | |||
z-index: 3; | |||
z-index: 4; |
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.
Have we tested the entire theme to ensure this doesnt break anything? 👀
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.
I looked at all "z-index
" css changes in the code and couldn't find any that should conflict with this change. I know you have context on the header section group changes that were made in the past, would you know what I could test/check for the header section group to ensure nothing is broken?
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.
I dont have any specific scenarios. I would test it with cart notification, search popup etc to ensure it doesn't break that experience. Since this changes the header globally we want to ensure it doesnt affect any existing features
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.
I noticed that when you scroll slowly, this happens: https://screenshot.click/07-00-kafue-59xri.mp4
[Edit by Andrew]: This is fixed.
assets/quick-order-form.css
Outdated
top: 0; | ||
z-index: 3; /* Prevent volume pricing popover overlap */ | ||
background-color: rgb(var(--color-background)); | ||
transition: top .15s ease-out; |
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.
For the duration, can we use one of our already defined lengths?
--duration-short: 100ms;
--duration-default: 200ms;
--duration-announcement-bar: 250ms;
--duration-medium: 300ms;
--duration-long: 500ms;
--duration-extra-long: 600ms;
--duration-extended: 3s;
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.
For context, I used what we currently have for .section-header.animate
:
Lines 2345 to 2347 in 2c84724
.section-header.animate { | |
transition: top 0.15s ease-out; | |
} |
But I'm curious if it's better to go with your suggested approach 🤔
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.
Makes sense. Yeah, I wonder if we can use an existing one for it, given its standardized. Let's see what @melissaperreault says
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.
Looking into this today!
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.
Im not seeing the subtotal bar 👀
https://screenshot.click/11-00-xwq2k-ydi01.png
[Edit from Andrew]: Fixed here: 5678872 (screenshot)
@eugenekasimov regarding your point here of when the ideal change should occur. @melissaperreault should the product change happen sooner? I'm also good with addressing this in a follow-up/polish task. |
Thanks Sofia! I've noted this as it was something @eugenekasimov and I noticed the other week 👍 Fixed here: 5678872 (screenshot) |
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.
On mobile, when the main header sticky is set to none
, this happens: https://screenshot.click/12-56-6seyh-j4uts.mp4
@@ -2745,6 +2745,9 @@ | |||
"quick-order-form": { | |||
"name": "Quick order form", | |||
"settings": { | |||
"enable_sticky_header": { |
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.
When items are added to cart, there is some jumping: https://screenshot.click/12-57-nfqnp-9n0sg.mp4
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.
Applying a set height
on thead
or thead tr
fixes this (video):
.quick-order-form .quick-order-list__table--sticky thead tr {
height: 7.2rem;
}
But then we have the issue of the thead
being this height when the sticky header isn't active (screenshot). I tried applying a sticky-active
class to the thead
when the sticky header is active and only apply the height this way but it still shifts up:
.quick-order-form .quick-order-list__table--sticky thead.sticky-active tr {
height: 7.2rem;
}
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.
I applied a set height here: 1274c92 temporarily to solve the upward shift but wondering if there's a better solution 🤔
@eugenekasimov what are your thoughts in applying a fixed/set height to prevent the upward shift after adding an item to the cart?
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 text is overlaying on top of the row: https://screenshot.click/08-13-8zf11-yis0j.mp4 👀
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.
Oops, I approved this accidentally 😅. Please, ignore my approval.
Remaining tasks
|
PR Summary
This PR adds a sticky header option to the quick order form.
Why are these changes introduced?
Fixes #3032 / Figma
The sticky header will be active on both desktop and mobile.
This quick order form sticky header should adjust accordingly with the different main sticky header settings in the header section (
None
,On scroll-up
default,Always
, andAlways, reduce logo size
).Other considerations
Translations
To save time, I manually added all translations for the "Enable sticky header" section setting so we don't have to request them. I got the translations from: 8aac1e5 when the main header section only had one setting:
enable_sticky_header
, before the 4 above settings were added. I marked translations as async.Important notes⚠️ 👀
The main focus for this PR is to add the sticky header functionality to the quick order form so that each and every product (screenshot) 'sticks' as you scroll down and up on the quick order form.
Header z-index adjustment
Since we're introducing an additional sticky header feature to the quick order form, it should play nicely with the existing, main sticky header. I increased the
z-index
ofshopify-section-group-header-group
to4
. This correctly addresses the volume pricing popover issue we previously encountered (screenshot), along with the issue where when you reach the bottom of the form and scroll back up (noted by Melissa).Testing steps/scenarios
Since the team is still ironing out implementation decisions, I currently have it set up to display the first 10 products from the
shop-all
collection.Make sure Enable sticky header is enabled.
Show variant images
Show SKUs
Demo links
Notes
This was built from the exploration work done here: