Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Introduce Checkout Form skeleton and Checkout Form Step Component #1329

Merged
merged 25 commits into from
Dec 6, 2019

Conversation

senadir
Copy link
Member

@senadir senadir commented Dec 5, 2019

This PR introduces the checkout form step component that handles showing and building steps.

Closes #1328

Things that are left to do:

  • Add PropTypes.
  • Change colors to variables.

Inspirations:
composite-checkout/checkout-step

Screenshots

Editor: StoreFront
image

Frontend: StoreFront
image

Editor: TwentyTwenty
image

Frontend: TwentyTwenty
image

Frontend: StoreFront - Mobile
image

Frontend: TwentyTwenty - Mobile
image

@senadir senadir added status: in progress focus: components Work that introduces new or updates existing components. labels Dec 5, 2019
@senadir senadir self-assigned this Dec 5, 2019
Copy link
Contributor

@nerrad nerrad left a 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.

assets/js/base/components/checkout/form-step/index.js Outdated Show resolved Hide resolved
@nerrad
Copy link
Contributor

nerrad commented Dec 5, 2019

@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: TimelineEvents (https://www.figma.com/file/Pfw5Qa1UOw03RiJere3Wlo/Automattic-Components?node-id=3182%3A1442)

@senadir
Copy link
Member Author

senadir commented Dec 5, 2019

@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 composite-checkout/checkout-step component naming.

@nerrad
Copy link
Contributor

nerrad commented Dec 5, 2019

Let's include inspiration for components etc (including links) in the pull description as well as that helps with review and rationale.

@senadir senadir marked this pull request as ready for review December 5, 2019 17:06
@senadir senadir requested a review from a team December 5, 2019 17:06
Copy link
Contributor

@Aljullu Aljullu left a 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.

assets/js/base/components/checkout/form-step/index.js Outdated Show resolved Hide resolved
assets/js/base/components/checkout/form-step/style.scss Outdated Show resolved Hide resolved
assets/js/base/components/checkout/form-step/style.scss Outdated Show resolved Hide resolved
assets/js/base/components/checkout/form/index.js Outdated Show resolved Hide resolved
assets/js/blocks/cart-checkout/checkout/block.js Outdated Show resolved Hide resolved
assets/js/blocks/cart-checkout/checkout/block.js Outdated Show resolved Hide resolved
assets/js/base/components/checkout/form-step/index.js Outdated Show resolved Hide resolved
assets/js/base/components/checkout/form-step/style.scss Outdated Show resolved Hide resolved
@senadir senadir requested review from nerrad and Aljullu December 5, 2019 22:09
@senadir
Copy link
Member Author

senadir commented Dec 5, 2019

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

Copy link
Member

@haszari haszari left a 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).

Screen Shot 2019-12-06 at 11 37 08 AM

Screen Shot 2019-12-06 at 11 37 54 AM

Paging @nerrad for thoughts on how to handle these - I'll leave the follow up/fix now decision in your hands :)

assets/js/base/components/checkout/form-step/index.js Outdated Show resolved Hide resolved
assets/js/base/components/checkout/form-step/style.scss Outdated Show resolved Hide resolved
assets/js/base/components/checkout/form-step/style.scss Outdated Show resolved Hide resolved
Copy link
Contributor

@nerrad nerrad left a 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.

assets/js/base/components/checkout/form-step/index.js Outdated Show resolved Hide resolved
assets/js/base/components/checkout/form-step/style.scss Outdated Show resolved Hide resolved
assets/js/base/components/checkout/form-step/style.scss Outdated Show resolved Hide resolved
Copy link
Contributor

@Aljullu Aljullu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for applying the changes @senadir, this is making good progress! I added a couple of comments more on top of what @haszari and @nerrad said.

@senadir
Copy link
Member Author

senadir commented Dec 6, 2019

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

@senadir senadir requested a review from nerrad December 6, 2019 11:50
Copy link
Contributor

@nerrad nerrad left a 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 :shipit:

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
focus: components Work that introduces new or updates existing components.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Form steps for Checkout block
5 participants