-
Notifications
You must be signed in to change notification settings - Fork 685
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
Checkout options #319
Checkout options #319
Conversation
Generated by 🚫 dangerJS |
`cartItemCount` defaults to 0 if `cart.details` is null or undefined
Test no longer uses inline snapshot for validation, and instead checks that a second request is made in response to a 404.
Do you mean in terms of dependencies, or just interactions between the two? As far as I'm aware, the changes involved were done in separate components. I also made these changes in a separate branch off of master, excluding all the changes from #260. |
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.
Thanks for the clarification. We're ready to merge this; please backmerge release/2.0
and resolve conflicts, and then we'll drop it in!
I was very excited about this PR, so I backmerged |
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.
Just a quick question about the CSS.
.active { | ||
position: absolute; | ||
transform: scale(1); | ||
transition: all 0.25s cubic-bezier(0.5, 1.8, 0.9, 0.8); |
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.
This is a really nice feeling transition. Maybe we should put it in a central place, so that other appear/disappear actions can have a similar feel.
Have you tested it on a low-powered device to make sure its framerate is ok?
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.
As far as devices go, it's only been tested using the Chrome performance tab. With 6x CPU slowdown and some artificial performance overhead, I didn't notice any significant difference (in terms of animations). If you have a target device in mind and want to take a look, the checkout options branch is live at https://upward.bargreen.io
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.
@zetlen Right now we're only using CSS custom properties when we want to store reusable values. Timing functions could certainly be stored as such, though.
@Serunde Consider limiting transition-property
to the properties being transitioned, rather than using all
. 😅
/* index.css */
html {
--venia-anim-bounce: cubic-bezier(0.5, 1.8, 0.9, 0.8);
}
/* kebab.css */
.myElement {
transition: transform 256ms var(--venia-anim-bounce);
}
My apologies for missing your earlier request to backmerge this PR! |
@jimbo Since there are UI innovations in this PR and you are back, I think it should get your review before we merge! But it's ready. |
.active { | ||
position: absolute; | ||
transform: scale(1); | ||
transition: all 0.25s cubic-bezier(0.5, 1.8, 0.9, 0.8); |
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.
@zetlen Right now we're only using CSS custom properties when we want to store reusable values. Timing functions could certainly be stored as such, though.
@Serunde Consider limiting transition-property
to the properties being transitioned, rather than using all
. 😅
/* index.css */
html {
--venia-anim-bounce: cubic-bezier(0.5, 1.8, 0.9, 0.8);
}
/* kebab.css */
.myElement {
transition: transform 256ms var(--venia-anim-bounce);
}
@jimbo New changes added to address your review, let me know if you have any other feedback! |
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.
@Serunde Thanks for addressing all my feedback! Just a couple small items left. 👍
Trim unnecessary styling in `kebab`
@Serunde Looks good now! Thanks for making changes. We just need to resolve conflicts and this will be set for merge. |
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.
Manually merged with work from #505. This is ready. 👍
This PR is a:
[x] New feature
[ ] Enhancement/Optimization
[ ] Refactor
[ ] Bugfix
[ ] Test for existing code
[ ] Documentation
Summary
When this pull request is merged, it will enable rudimentary remove, edit, and 'add to favorites' actions to Venia items inside the shopping cart. This PR also includes the relevant Storybook configuration required for running its UI tests.
Addresses #265
Additional information
New components:
These changes contain a workaround for a discovered issue where the last item removed from a guest cart puts the cart in an invalid state, and the next item added to this empty cart has a price of zero. Now, when the last item is removed, the cart is reset.
REST/GraphQL support for the 'add to favorites' feature is currently insufficient.