-
Notifications
You must be signed in to change notification settings - Fork 683
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
Venia v2 Cart Page #2084
Venia v2 Cart Page #2084
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.
Nice work @supernova-at
A question/suggestion. Other than that, no issues.
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 only thing I really want to see fixed is the css for the order summary, otherwise this is good.
* TODO: Use CSS Properties (variables) or something instead of hardcoding these. | ||
* min-height of nav header + page spacing + height of page header | ||
*/ | ||
top: calc(3.5rem + 3.5rem + 25px); |
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 not sticking as I would expect. Add some extra long text like lorem ipsum and scroll. You'll see that it scrolls off the page.
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.
Oh yeah, that's what
Scroll and verify that the "Order Summary" container "sticks" to the side / stays in view. Note that it is expected that it will disappear once you scroll past all of the content.
was supposed to identify 😛.
If we're talking about the same thing, the fact that you can scroll past the content entirely is a product of a style that gets applied to all pages: the initial height of the page content area is always set to 100vh
.
I think once the components on this page are present we won't be able to repro this.
|
||
.sign_in { | ||
font-weight: bold; | ||
text-decoration: underline; |
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 text looks a little larger than in the mocks, at least in relation to its counterpart in the header, "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.
That's actually a good callout - I don't know how to open the mock in XD to get the correct size. I'll dig deeper.
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.
Should be able to find a link somewhere in the epic or linked issues. But @tjwiebell is also having trouble finding pixel/size direction from XD so not sure how to help from there.
grid-area: adjustments; | ||
} | ||
|
||
.recently_viewed_container { |
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.
Since this is an extension/dynamic location can we just call this cart_extension_container
and rename the grid area?
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.
Do we know for sure that's the case? I wasn't clear.
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.
Ah yea, it was discussed when we broke the stories down that its more like a configurable area for things like "recently viewed" or "people like you purchased..." etc.
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.
TBH It may be better to simply remove/comment out the html since those components don't exist and I'm not sure we are expected to even implement it within this epic.
*/ | ||
top: calc(3.5rem + 3.5rem + 25px); | ||
top: 3.5rem; |
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.
Would have liked this to be a variable but the TODO will suffice
Looking through late on this one, but it's fine. We can patch up the CSS as part of the pricing summary work. 👍 |
Description
Venia v2 separates out the Cart into its own page. This PR lays out the basic structure of the page. Future PRs will populate the page with its actual content / components.
Related Issue
Closes PWA-130.
Acceptance
Verification Stakeholders
@jimbo
Specification
Verification Steps
yarn watch:venia
<your dev site>/cart
640px
) viewsOn Mobile:
On Desktop:
Screenshots / Screen Captures (if appropriate)
Mobile
Desktop
Checklist