-
Notifications
You must be signed in to change notification settings - Fork 219
Introduce Checkout Form skeleton and Checkout Form Step Component #1329
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.
This is looking good so far. I do have a question about <SecondaryAction/>
and how that will be used and there's the outstanding question about colors that we've discussed in slack.
@Aljullu Should we be following the Component naming and schema more closely with the Figma design? For example, the base component for the steps is: |
@nerrad Naming is inherited from the Automattic Component library but it’s not relevant to this component since it’s heavily modified to resample a checkout, I tried to follow the |
Let's include inspiration for components etc (including links) in the pull description as well as that helps with review and rationale. |
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.
Awesome, this is looking great @senadir! Some feedback/comments below.
Co-Authored-By: Albert Juhé Lluveras <aljullu@gmail.com>
…oocommerce-gutenberg-products-block into add/form-steps-to-checkout
I guess this is ready for a second review, we can always iterate on this in the future as long as we have a base for further PRs |
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.
Code looks good, I don't see any blockers there, have added some comments.
I tested with some responsive modes with Chrome & Safari, and also tested with Twenty Twenty as well as Storefront, and found some issues. Some of these are probably blockers, others could be handled as follow up.
- Step text is truncated on mobile, needs to wrap. I'm thinking we get this layout working reasonably on mobile now, but happy to defer that til we have more fleshed-out markup.
- Steps look a little strange in Twenty Twenty. Again, we could defer this but I think it's healthy to get the containers/layout functioning well in a range of environments now (default theme x mobile).
Paging @nerrad for thoughts on how to handle these - I'll leave the follow up/fix now decision in your hands :)
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've commented on things that should be addressed inline with the code.
@haszari thanks for the cross browser/theme and viewport size testing. @senadir, I think we should address his observations in this pull. I realize that this will be part of a larger container in the finished copy but I think it's still important to get it working reasonably well in various mobile views now (in keeping with the design mockup) and also address any potential resets needed to ensure cross theme consistency.
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.
Another round of review and another round of commits, I hopefully fixed all the mentioned issues and some compatibility issues, I also opened a PR to handle theme things for the future #1334 I also updated the screenshots to the current state, feel free to give them a look and do another round of review |
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 double-checked against Safari, mobile views, StoreFront, Twenty Twenty, and Twenty Nineteen and it looks good! Let's
This PR introduces the checkout form step component that handles showing and building steps.
Closes #1328
Things that are left to do:
PropTypes
.Inspirations:
composite-checkout/checkout-step
Screenshots
Editor: StoreFront
Frontend: StoreFront
Editor: TwentyTwenty
Frontend: TwentyTwenty
Frontend: StoreFront - Mobile
Frontend: TwentyTwenty - Mobile