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.

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

feat: add Orders to Account Profile #507

Merged
merged 57 commits into from
Mar 8, 2019

Conversation

kieckhafer
Copy link
Member

@kieckhafer kieckhafer commented Feb 27, 2019

Expands upon #499 to add Orders data to profile page
Impact: minor
Type: feature

Issue

Add Orders data to Account Profile

Solution

  • Split existing orders page into profileAddressBook and profileOrders pages. This is necessary due to the way we do pagination, our existing method broke when using asPath to display different components (Orders or AddressBook) on a single page based on the URL.
  • Add 5 and 10 page size to our default pagination. Since we use the same component for CatalogItems / Tags, and now orders, and these return various dimensions of results we should allow more than just 20,60, or 100 for the results, as 20 is far too many OrderCard's to scroll through
  • Update Thank You page after an order is placed to use OrderCard, with the Header portion expanded by default (it's not expanded by default on the profile page).

Breaking changes

None

Testing

  • Visit /profile/orders as a non-logged in user, and see a 404 / page not found
  • Login as a user, create 6+ orders (to see pagination)
  • Visit /profile/orders as a logged in user, see that orders show up and match our designs: https://zpl.io/V0ypYWR
  • Move orders into different workflow statuses
  • See orders are filtered as they should

Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
…ent Library updates

Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
…gs (orders)

Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
kieckhafer and others added 4 commits February 26, 2019 23:32
Signed-off-by: Erik Kieckhafer <ek@ato.la>
…cross app

Signed-off-by: Erik Kieckhafer <ek@ato.la>
…commerce/reaction-next-starterkit into feat-kieckhafer-newAccountPage
Copy link
Contributor

@nnnnat nnnnat 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 great. I did notice this copy issue with the order status labels.
order-process-status

This happens for me with all the status except "new"

@kieckhafer
Copy link
Member Author

@nnnnat Yes, that is expected if you haven't added the translations data. Instructions are here: reactioncommerce/reaction#4981

Let me know if that fixes the issue. "new" is probably also wrong, it's probably not capitalized in your version. If you add them to the settings it'll change.

@nnnnat
Copy link
Contributor

nnnnat commented Feb 28, 2019

@kieckhafer that makes sense I'll retest in a bit

@aldeed
Copy link
Contributor

aldeed commented Mar 6, 2019

@kieckhafer Regarding the copy issue Nat mentioned, it's still possible that people could eventually have long status labels when we allow customizing them. Can we do something with CSS, maybe overflow-wrap: break-word;, on the status badge to prevent it overlapping the date? cc @cassytaylor

aldeed
aldeed previously requested changes Mar 6, 2019
Copy link
Contributor

@aldeed aldeed left a comment

Choose a reason for hiding this comment

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

Mostly looks good. Just a couple small suggestions.

@@ -48,6 +48,7 @@
"compression": "^1.7.3",
"cookie-parser": "^1.4.3",
"cookie-session": "^2.0.0-beta.3",
"date-fns": "^1.30.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Was there a decision to use date-fns instead of moment for new projects? It's fine with me but I'd like to see the decision recorded and communicated if it hasn't been.

Copy link
Member Author

Choose a reason for hiding this comment

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

We had a chat in Slack about a light(er)weight solution for dates, considering we need very little. Made a DR and tagged you in Notion.

src/components/OrderCard/OrderCard.js Outdated Show resolved Hide resolved
}

onTrackShipmentButtonClick() {
// TODO: What do we do to track a shipment? Link to a specific provider website?
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'll add trackingUrl prop on fulfillmentGroup in my tracking PR, and then you can render that in a link here? Seems better than recording the carrier and trying to dynamically build links to different carriers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that sounds like a good idea.

@kieckhafer kieckhafer requested a review from aldeed March 6, 2019 23:51
@kieckhafer kieckhafer dismissed aldeed’s stale review March 6, 2019 23:52

@aldeed updated based on comments, ready for a look

@kieckhafer
Copy link
Member Author

kieckhafer commented Mar 6, 2019

@aldeed did not update the long name on the Badge yet. Created an issue here to have design take a look since it shouldn't be a blocker: #515

Signed-off-by: Erik Kieckhafer <ek@ato.la>
@kieckhafer kieckhafer requested a review from nnnnat March 8, 2019 18:45
@kieckhafer kieckhafer merged commit f2eb972 into develop Mar 8, 2019
@kieckhafer kieckhafer deleted the feat-kieckhafer-newAccountPage branch March 8, 2019 20:17
@jeffcorpuz jeffcorpuz mentioned this pull request Mar 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants