[PWA-181] Venia checkout skeleton

[revanth0212]

Description

Venia checkout page skeleton. This PR creates the file system and the basic state needed to render the checkout page.

In this PR I have created a new utility called renderIf. It is a new methodology in our code base. Let me know what you guys think of it.

Related Issue

Closes PWA-181

Verification Steps

1. Go to /checkout on the deployed app to see the checkout page.

Screenshots / Screen Captures (if appropriate)

Desktop Checkout Page

Desktop Confirmation Page

Mobile Checkout Page

Mobile Confirmation Page

Checklist

None

[PWAStudioBot]

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖	Associated JIRA tickets: PWA-181.

Generated by 🚫 dangerJS against 1e66b1a

[sirugh]

This was an unspoken thing with the cart page but we only want to use cartId state from cart context in the new cart and checkout pages. The reason for this is that state, such as items and prices, are now maintained entirely in apollo's cache. Instead of checking context for this, use a query for total_quantity.

[revanth0212]

Good to know. For now, I will leave it to the individual working on the ticket to make the changes. The intent of this PR is to scaffold checkout.

[sirugh]

I'm not sure there's a follow-up ticket for checkout refactor but I definitely don't want to scope creep you in this ticket. I'll make a ticket and reference this comment.

Made this: https://jira.corp.magento.com/browse/PWA-395

[sirugh]

One way to tell if shipping information/method/etc are done is to just make a query for the data and see if anything returns. In the case of shipping information we will probably want to do something like query for shipping address as the estimator on cart does but then wipe the cache of any of that fake data.

I'd definitely prefer that we allow the apollo cache to control the "renderable" state of things. For example:

• checkout page would query for shipping information (along with other info such as shipping method, pre-added gift card/coupons/etc) which dictate what step to display.
• shipping address component would query for any provided cart shipping address which, if found, would tell it to render in the "card" UI rather than the form UI.

As it is, on a page refresh you lose local state which means you would have to start over each time when in reality part of your data would have already been saved to gql.

[revanth0212]

You are absolutely right about losing data on refresh which is not what we want. We can use apollo client local queries for that. Also, these boolean flags here are used to track progress, not data presence. Even if you have shipping method picked in cart page, and that data is available at the time of checkout, you still need them to click on proceed to payment information but use the data from prev step or from the account (shipping address for instance). This is what I understood after I spoke with @soumya-ashok a couple of days ago. That is the reason I have those progress indicator flags. They have to be moved to a persistent store, but for now, since this is just the scaffolding PR, I have left them in the hook.

[sirugh]

Also, these boolean flags here are used to track progress, not data presence.

Right, I'm just implying that data presence itself is an indicator of the progress a user has made so there shouldn't be a need to re-submit it, which is what those "proceed to X" buttons are for.

Assume a user has entered (and saved to the server) a bunch of information on their cart and they're on some final step like payment method. They've clicked "proceed to X step" a bunch of time at that point. If they refresh the page, for whatever reason, they will have to click through all those steps again, likely resubmitting redundant data that the server is already aware of.

I'd love to see if a sort of "auto-progression" to the latest unfilled step, along with a "back button" would be something @soumya-ashok or @schensley are interested in for this.

Either way, local state is fine for now and I can add a line item to PWA-395.

[revanth0212]

You are right. Let's see what Soumya and Scott have in mind.

[sirugh]

This is a really interesting approach to conditional rendering. Currently, throughout the app, we have code that uses memoization that returns the component. This allows us to also maintain readability in the JSX markup. Example:

const maybeRenderComponent = useMemo(() -> {
  if (condition) {
    return <SomeComponent />
  }
  else {
    return null;
  }
}, [condition])

return (
  <div>
    {maybeRenderComponent}
    <SiblingComponent />
    <span>Some text</span>
  </div>
);

This is all to say that while I think this is a neat idea, I'm not sure we want to introduce this approach. @jimbo will likely have input here.

[jimbo]

@sirugh is correct. As we've discussed a few times before, in this codebase, we avoid utilities. The only utility our components use is mergeClasses (because we have to), but even that one is something we're aiming to eliminate.

I've worked in codebases where utility code was common. I've also used Ramda to create some beautiful functional code. Ultimately, that approach is not the right one for this project, and more importantly, it's not the one we selected in the first place. As a general rule of thumb, please write code that looks like the other code in the codebase. 😅

[sirugh]

Instead of using this conditional render, this should be a lazy query, triggered off cart_id, for total_quantity. Check out how CartPage does it.

[revanth0212]

I thought of leaving that to the individual taking up the respective checkout tickets because it feels like an implementation detail. In this PR I am trying to scaffold checkout and add bare bone state to get the checkout flow working.

[sirugh]

Yea, I just thought this was relevant to the initial bare bone state :P Regardless, made a ticket to follow up.

[jimbo]

@sirugh is correct. As we've discussed a few times before, in this codebase, we avoid utilities. The only utility our components use is mergeClasses (because we have to), but even that one is something we're aiming to eliminate.

I've worked in codebases where utility code was common. I've also used Ramda to create some beautiful functional code. Ultimately, that approach is not the right one for this project, and more importantly, it's not the one we selected in the first place. As a general rule of thumb, please write code that looks like the other code in the codebase. 😅

[sirugh]

One issue I see with this approach is the ambiguity of steps. Looking at step number it is hard to grasp what step that number belongs to. It would take more effort If someone wants to change or remove a certain step.

Right, the difference in that regards from the example code and your code with the renderIf function was that I defined the condition within the memo rather than outside. The step/memo is self-aware of its render condition.

[sirugh]

I made https://jira.corp.magento.com/browse/PWA-406 but I didn't know what to include. I assume you and @soumya-ashok discussed this, or maybe you saw something in the epic.

Either way, please update this text and let @awilcoxa know so that he can get details for the story.

[revanth0212]

There was no ticket to create this component

I added my notes to the ticket.

[sirugh]

I know there was no ticket, so I made one :D

My ask here is that you update the text to reflect the new ticket as you have done for the other "to be completed by PWA-X" sections.

[sirugh]

Empty cart feels sparse. Surprised there isn't a call to action like "Continue Shopping".

[revanth0212]

This was never part of the XD or the epic. So I added filler text. UX needs to come up with a design for this.

@soumya-ashok @schensley let me know how to proceed.

[sirugh]

Even though we are using the redux actions for create cart (as there isn't a better alternative now) I think that fetching data for the checkout page should be done identically to how the cart page does it.

Create a lazy query and call it in an effect whenever the cartId changes (so long as there is a cartId).

See https://github.com/magento/pwa-studio/blob/develop/packages/peregrine/lib/talons/CartPage/useCartPage.js#L13-L34

[revanth0212]

Like I mentioned previously, I'll leave this to the individual working on the ticket. The scope of this PR is to set up a basic state and UI.

[sirugh]

While we use the [state, api] return for context hooks, no other talons use this API that I'm aware of.

I really do like the idea, but for now maybe we should stick to the norm? What are your thoughts @jimbo? I feel like this makes the talon APIs much more resistant to breakage.

Edit: I misspoke - we do already return an object from talons. I had mixed up function args and return values. Either way I am still interested in this pattern since it matches existing hook return patterns.

[revanth0212]

Yeah, I was confused of this as well. I personally feel [state, api] is a cleaner structure. I might be wrong but react (un-officially) pushes that return structure for react hooks (both built in and custom).

[jimbo]

Yeah, I was confused of this as well. I personally feel [state, api] is a cleaner structure. I might be wrong but react (un-officially) pushes that return structure for react hooks (both built in and custom).

You're correct, React does favor a [state, api] return signature in its own hooks. That also happens to be the reason why we selected that signature for our context hooks, such as useAppContext. It's a very intuitive structure.

Talons are a bit different though, so we took them in a different direction. They're more like components, in that they can do pretty much anything; they can hold state, define callbacks, and perform side effects. The only thing they can't do is create elements. In order to not confine them, and to ensure they don't appear to be generic or reusable, we gave them the most basic, consistent API possible:

propsObjectIn => propsObjectOut

It's not that talons are incompatible with a [state, api] signature, though; it's that it's too late to change the pattern (for now). In the future, we can revisit the talon APIs as a whole and consider whether to apply more constraints to them or reshape the signature. Just now, though, adoption of this new feature will depend on consistency, so let's stick to the existing pattern. Object in, object out, please.

[sirugh]

Because we have named areas here for potentially hidden nodes there is some extra blank space created by the grid:

I think you may be able to replace the names with .. The structure should be the same but it will compress the blank spaces.

[revanth0212]

UI has changed. This is not relevant anymore.

[jimbo]

In general, we should avoid defining areas explicitly with grid-template-areas. It has its situational uses (the product page, for example), but for most layouts, we should prefer to define only the tracks and allow the content to either place itself into the grid or rely on the automatic placement algorithm. The result will be a much more flexible, resilient layout.

That said, this actually would have been a potential case for grid-template-areas, or at least for an explicit grid. Your use case involves one grid item spanning all rows, and since that's not possible without making the grid explicit, grid-template-areas was a solution. Another solution might have been to wrap the left-column grid items in a single element.

Fortunately, the summary is actually sticky, so you don't need to make the grid areas explicit. Instead, try the following:

.body {
  grid-template-columns: 2fr 1fr;
}

.summary_container {
  grid-column: 2 / span 1;
  grid-row: 1 / span 1;
  /* ensure this element doesn't contribute to the row height */
  height: 0;
  /* position sticky should be here, not on its child */
  position: sticky;
}

.others {
  grid-column: 1 / span 1;
}

[sirugh]

Just noting that I don't personally like inline conditional renders but I'm guessing you and @jimbo already talked about it so I leave it to you guys to hash out.

[revanth0212]

I am not sure if we have a standard for this, but I found both if-else and && for conditional renders in our code base.

[brendanfalkowski]

FWIW, we settled on the more explicit ternary syntax like:

{isAlive ? ( <Thing /> ) : null}

Over the shorter but less familiar (I don't even know what this syntax is called):

{isAlive && ( <Thing /> )}

[revanth0212]

There is no name as such in react but && or || are short circuit evaluation elements in CS. So I believe we can call it short circuit rendering.

[jimbo]

I've covered this subject a number of times in various PRs, but I suppose the real problem is that the pattern isn't clear because we haven't been perfectly consistent in the codebase. 😅

1. Avoid multiline statements in JSX expressions. The main reason to use JSX is because trees are very readable (like XML).
2. Avoid short-circuiting JSX elements with &&. The alternative to a JSX element should be null—not undefined, not false, and not anything else. As Brendan said, use a ternary to be explicit about it.

I've opened #2200 to correct all of the cases in the codebase where we were inconsistent about this.

[jimbo]

Great work. Good eye for typography and space; it looks very consistent with the app.

[jimbo]

Be sure not to define more than one component per file. For each of these components other than CheckoutPage, create a new file in the CheckoutPage directory.

components
  CheckoutPage
    checkoutPage.js
    guestCheckoutOptions.js
    index.js

If these components are private (which it looks like they are), then simply don't re-export them in index.js.

[revanth0212]

These are private components used just in the CheckoutPage component. I am not exporting them in the index.js file.

[jimbo]

In general, we should avoid defining areas explicitly with grid-template-areas. It has its situational uses (the product page, for example), but for most layouts, we should prefer to define only the tracks and allow the content to either place itself into the grid or rely on the automatic placement algorithm. The result will be a much more flexible, resilient layout.

That said, this actually would have been a potential case for grid-template-areas, or at least for an explicit grid. Your use case involves one grid item spanning all rows, and since that's not possible without making the grid explicit, grid-template-areas was a solution. Another solution might have been to wrap the left-column grid items in a single element.

Fortunately, the summary is actually sticky, so you don't need to make the grid areas explicit. Instead, try the following:

.body {
  grid-template-columns: 2fr 1fr;
}

.summary_container {
  grid-column: 2 / span 1;
  grid-row: 1 / span 1;
  /* ensure this element doesn't contribute to the row height */
  height: 0;
  /* position sticky should be here, not on its child */
  position: sticky;
}

.others {
  grid-column: 1 / span 1;
}

[jimbo]

Likewise here, let's prefer to let summary handle its own responsive placement.

/* at mobile resolutions */
.summary {
  grid-column-start: 1;
  grid-row: unset;
  height: auto;
}

[revanth0212]

I followed the usage of grid css in the cart page. It had a similar layout. Thought that is how we will be doing css. Ok, will look into this.

[jimbo]

In this codebase, classnames are never shared between two elements. This element, for example, doesn't know anything about its presentation:

• what styles are associated with the classname classes.text?
• how is its presentation similar to or different from its sibling above?

Since it doesn't know any of these things, the only purpose of giving it a classname is to uniquely identify this element within the component. In this way, it's more like an ID than a classname. The CSS file can choose to apply some styles to this classname (or not), and it can choose to use composes to avoid repeating itself (or not).

In brief, choose unique, short, descriptive classnames for each element.

[revanth0212]

Good to know. I will do that.

[jimbo]

Yeah, I was confused of this as well. I personally feel [state, api] is a cleaner structure. I might be wrong but react (un-officially) pushes that return structure for react hooks (both built in and custom).

You're correct, React does favor a [state, api] return signature in its own hooks. That also happens to be the reason why we selected that signature for our context hooks, such as useAppContext. It's a very intuitive structure.

Talons are a bit different though, so we took them in a different direction. They're more like components, in that they can do pretty much anything; they can hold state, define callbacks, and perform side effects. The only thing they can't do is create elements. In order to not confine them, and to ensure they don't appear to be generic or reusable, we gave them the most basic, consistent API possible:

propsObjectIn => propsObjectOut

It's not that talons are incompatible with a [state, api] signature, though; it's that it's too late to change the pattern (for now). In the future, we can revisit the talon APIs as a whole and consider whether to apply more constraints to them or reshape the signature. Just now, though, adoption of this new feature will depend on consistency, so let's stick to the existing pattern. Object in, object out, please.

Approved