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

Update order detail page #762

Merged
merged 7 commits into from
Jan 18, 2019
Merged

Update order detail page #762

merged 7 commits into from
Jan 18, 2019

Conversation

jimbo
Copy link
Contributor

@jimbo jimbo commented Jan 17, 2019

This PR is a:

  • New feature
  • Enhancement/Optimization
  • Refactor
  • Bugfix
  • Test for existing code
  • Documentation

Summary

When this pull request is merged, the order detail UI will be more legible, use the new button styles, and have simpler tests.

@vercel
Copy link

vercel bot commented Jan 17, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@coveralls
Copy link

coveralls commented Jan 17, 2019

Coverage Status

Coverage increased (+0.3%) to 57.808% when pulling a3b6252 on jimbo/buttongroup into 3e19bbe on release/2.0.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 57.711% when pulling 4008025 on jimbo/buttongroup into a11d637 on release/2.0.

Copy link
Contributor

@supernova-at supernova-at left a comment

Choose a reason for hiding this comment

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

Works great, two minor feedbacks.

onBuyAgain={onBuyAgain}
onShare={onShare}
/>
<div className={classes.list}>{props.children}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove "Buy Again" and "Share" from the "Interactions" section in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I should just add this functionality back. It looked like it was just mocked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After further review, it basically is mocked. The share logic is a total hack, and the add-to-cart functionality doesn't work since all the data is mocked. Going to stub things out and write a couple tests. I think the docs can stay unchanged that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. 👍

@sirugh sirugh self-requested a review January 18, 2019 15:26
Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

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

I'm always hesitant to approve without having found anything but nothing really jumped out to me. I assume the design is based on mocks somewhere but without having seen them everything looks OK to me.

@jimbo jimbo merged commit 383a37e into release/2.0 Jan 18, 2019
@jimbo jimbo deleted the jimbo/buttongroup branch January 18, 2019 18:38
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.

4 participants