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

Sb es modal test #44

Closed
wants to merge 14 commits into from
Closed

Sb es modal test #44

wants to merge 14 commits into from

Conversation

espain16
Copy link
Contributor

@espain16 espain16 commented May 21, 2020

Type of Changes

Type
πŸ› Bug fix
βœ“ ✨ New feature
πŸ”¨ Refactoring
πŸ’― Add tests
πŸ”— Update dependencies
πŸ“œ Docs

Related Issue

closes #15

Description

From the shopping list, the user can tap to navigate to a detailed view of the item.
The detail view includes the following information:

  • Name of the item
  • Last purchased date
  • Estimated number of days until the next purchase
  • Number of times the item has been purchased"
    When in the β€œitem details” view, a back arrow is displayed in the header to take the user back to the β€œlist” view.

List To Complete Task

  • Create a component called ItemDetails.
  • Create a route for Item-Details.
  • Add link to each item to view the item's details.
  • Add a back arrow on the details page to take the user back the Shopping list.
  • Format the last purchased date to be human readable.

Updates

Before

Previous Shopping List

Screen Shot 2020-05-19 at 11 32 26 AM

After

Update To Shopping List UI

Screen Shot 2020-05-19 at 11 28 34 AM

Item Detail View

Screen Shot 2020-05-21 at 10 44 23 AM

Acceptance Criteria

  • From the shopping list, user can tap to navigate to a detailed view of the item
  • Detail view includes the following information:
    • Name of the item
    • Last purchased date
    • Estimated number of days until the next purchase
    • Number of times the item has been purchased"
  • When in the β€œitem details” view, a back arrow should appear in the header that takes the user back to the β€œlist” view

Testing Steps / QA Criteria

Click Here for the preview of our branch.

  • Click view details
  • Click back to shopping or the close button to return to list view.

@@ -0,0 +1,235 @@
/** Modal Styling **/
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would probably include a comment linking to the source of this CSS

Copy link
Contributor

Choose a reason for hiding this comment

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

True, true. I just forgot.

Copy link
Collaborator

@stevelikesmusic stevelikesmusic left a comment

Choose a reason for hiding this comment

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

Looks great going with a different approach than the first PR πŸ‘

It'd be nice if we can still connect to router for the modal. I pushed a branch modal-to-router that does just this. Give it a peek if you get stuck syncing the two.

</button>
<button onClick={handleShow}>View Details</button>
</li>
<TestModal item={item} show={show} handleClose={handleClose} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is great! Nice way to show the item details. Another option using a different library might be Drawer from Material UI

let lastPurchase = item ? new Date(item.last_purchased).toDateString() : '';

return (
<>
Copy link
Collaborator

Choose a reason for hiding this comment

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

You shouldn't need the Fragment if you have a wrapping component like Modal.

return (
<>
<Modal show={show} onHide={handleClose}>
<Modal.Header>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice use of Modal's compound components πŸ‘

Copy link
Contributor

Choose a reason for hiding this comment

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

πŸ”₯

<button
className={isPurchased ? 'purchased' : 'not-purchased'}
onClick={onHandle}
disabled={isPurchased ? !null : null}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not from this PR, but this expression can be simply disabled={!isPurchased}.

Suggested change
disabled={isPurchased ? !null : null}
disabled={!isPurchased}

Copy link
Contributor

@SaraSweetie SaraSweetie left a comment

Choose a reason for hiding this comment

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

This works well. It's up to you which you choose. I like either version!

<Shopping userToken={userToken} list={data} />
)}
/>
<Route
Copy link
Collaborator

Choose a reason for hiding this comment

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

The /add route needs to go first @espain16 @skillitzimberg. Switch will match the first path that it matches.

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.

13. As a user, I want to view details of my purchases to better understand my purchase patterns
5 participants