Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[CHEC-1009] Customer account page #149

Merged
merged 7 commits into from Nov 21, 2020
Merged

[CHEC-1009] Customer account page #149

merged 7 commits into from Nov 21, 2020

Conversation

kvisca
Copy link
Contributor

@kvisca kvisca commented Nov 18, 2020

Adding a customer's account page with an individual order view page.

Account Page
Screen Shot 2020-11-18 at 4 27 02 PM

Order Page
Screen Shot 2020-11-18 at 4 26 45 PM

@linear
Copy link

linear bot commented Nov 18, 2020

CHEC-1009 Build a customer's home page

After a user has logged in they'll be sent to this page. We can show some super basic information such as:

  • Their name and email address
  • A list of their orders

These are available from the following APIs:

  • GET /v1/customers/{customer_id}
  • GET /v1/customers/{customer_id}/orders

Both of those APIs require knowledge of the customer ID, which is returned in the login handler (CHEC-1008), and the customer JWT which is also returned in that task.

We can probably store these in session storage or something… cc @guy

@robbieaverill
Copy link
Contributor

A couple of minor things from your screenshots: can you use sentence casing, and your dates don't look like they're being parsed correctly

@kvisca kvisca force-pushed the feature/customer-account-page branch from 334a826 to e4b459b Compare November 18, 2020 21:48
@robbieaverill
Copy link
Contributor

I've rebased locally this onto #152 in order to test it because we broke things. Some feedback:

  • We should probably use the customer_reference attribute here instead of the order ID e.g. CMMRC-120465. Also applies to the page title. IDs are more developer focused where customer references are more consumer focused.
    image
  • If you hit /login when you're already logged in it should redirect you to /account, it doesn't at the moment
  • Billing address (and shipping while you're at it) needs to handle parts of the object that are null:
    image
  • The billing and shipping cards are pretty close together. Suggest adding pb-4 or something to the top card so it spaces out a little better
  • This page is missing a "< Back to My Account" breadcrumb link at the top I think. We have a design pattern for this in the checkout you could probably re-use:
    image

There are also a couple of console errors on this page for me:

image
image

I don't think this is related to this PR specifically, but I think we should change the "Logout" link in the top right header when you're logged in to "Account" which sends you to your account, then put the Logout link somewhere on the account page:

@robbieaverill
Copy link
Contributor

Hey @kvisca, still a few things:

  • Billing address undefined
    image
  • Sentence casing
    image
    image
    image
  • Clicking logout throws an error:
    image
  • Error parsing date:
    image

Functionally, I notice that every click from "My account" to an order, back again, to another order etc, reloads everything in Redux (categories, products, cart). This is definitely not ideal.

Good news is this looks good on mobile!

@kvisca
fixing missed casing, fixing logout error, adding logout message, add…
2734323 
…ed date generators to check for invalid dates.
@kvisca
Copy link
Contributor Author

kvisca commented Nov 20, 2020

@robbieaverill Thanks for the feedback, should be good to go now. Let me know if you catch anything else!

@robbieaverill
Copy link
Contributor

@kvisca:

  • Change to "My account":
    image
  • Change to "Ordered on" or "Order placed on" or something:
    image
  • Do you think this screen needs something like a "continue shopping" button or something to take you somewhere?
    image

I haven't reviewed the code at all yet, only UI so far.

@kvisca
Copy link
Contributor Author

kvisca commented Nov 20, 2020

I haven't reviewed the code at all yet, only UI so far.

Thanks, @robbieaverill. I've updated as requested.

@robbieaverill
Copy link
Contributor

Still getting this:
image

customerAccountPage has customer: {} in props. Otherwise I can't see where this would come from.

This shows when I first log in. I can navigate into an order view and back again and it will still show. When I refresh the page it renders the date correctly. At that point the customer object in props contains my information.

Functionally, I notice that every click from "My account" to an order, back again, to another order etc, reloads everything in Redux (categories, products, cart). This is definitely not ideal.

This is still happening

robbieaverill
robbieaverill reviewed Nov 20, 2020
View reviewed changes
Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

Lots of these comments are about linting. Please make sure you lint your code (yarn lint).

pages/account/[id].js
import moment from 'moment';
import { useSelector } from 'react-redux'

export default function SingleOrderPage() {
Copy link
Contributor

Choose a reason for hiding this comment

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

The rest of this repository uses components, it feels weird to me to move away from that in one place. Can you please refactor this so it uses components instead?

pages/account/[id].js
useEffect(() => {
const fetchOrderById = async (id) => {
try {
const order = await commerce.customer.getOrder(id, customer.id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to store these in Redux instead, so they'll persist between pages and avoid refetching?

pages/account/[id].js
setData(order.data);
} catch (err) {
setLoading(false);
setError(err?.message);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this operator supported enough to use yet? If our transpiler supports it then all good, otherwise it's not supported in IE which is a good reason not to use it in this repository

pages/account/[id].js Outdated Show resolved Hide resolved
pages/account/[id].js Outdated Show resolved Hide resolved
pages/account/index.js Outdated Show resolved Hide resolved
pages/login.js Outdated Show resolved Hide resolved
style/scss/helpers/_utility-classes.scss Outdated Show resolved Hide resolved
style/scss/pages/_account.scss Outdated Show resolved Hide resolved
yarn.lock Show resolved Hide resolved
@kvisca
Copy link
Contributor Author

kvisca commented Nov 20, 2020

Lots of these comments are about linting. Please make sure you lint your code (yarn lint).

Just so you're aware, yarn lint doesn't catch any of this on this project.

@robbieaverill
Copy link
Contributor

Cool, I'll update the linting rules later. In the meantime please take advantage of the human linting results above 😄

@kvisca
Copy link
Contributor Author

kvisca commented Nov 20, 2020

Cool, I'll update the linting rules later. In the meantime please take advantage of the human linting results above 😄

Much appreciated 👍

@robbieaverill robbieaverill merged commit 8ef52a6 into master Nov 21, 2020
@robbieaverill robbieaverill deleted the feature/customer-account-page branch November 21, 2020 00:10
@robbieaverill
Copy link
Contributor

Nice work. We can address any outstanding issues in new PRs in future 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robbieaverill robbieaverill robbieaverill left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@kvisca @robbieaverill