-
Notifications
You must be signed in to change notification settings - Fork 0
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
Resolves Issue #13 (#35) - View Purchase Details #56
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
src/App.js
Outdated
@@ -8,6 +8,8 @@ import getToken from "./lib/token"; | |||
import Form from "./Form/Form"; | |||
import * as firebase from "./lib/firebase"; | |||
|
|||
// heck spiky rake | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unnecessary comments!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOL that was mine and I'm so sorry I keep forgetting about it and it never dies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that everything here looks good code wise. This is probably my favorite PR description thus far! I liked your use of screenshots and the detailed description. I was able to navigate the code changes easily because of this. Great work!
import { NavLink } from "react-router-dom"; | ||
import "firebase/firestore"; | ||
import * as firebase from "../lib/firebase"; | ||
import classes from "./List.module.css"; | ||
import Modal from "../Modal/Modal"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like that you all used Modal - I think it makes for a cleaner UI experience.
Everything looks great and I definitely like using the modal to view details. Also, this has definitely been a lesson in PR requests! Thanks for being so thorough! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent! 🏅
Issue: #35
Tasks:
The main purpose of the issue was to create a way for the user to view the details of their purchase to better understand their purchase patterns. We opted to use a modal to display the item details, which is opened by clicking a button, which is found alongside each item on the list.
Actions:
Notes:
PR appears to mistakenly also include the changes made as part of a previous issue
Screenshots:
EXAMPLE 1 - VIEW DETAILS BUTTON
EXAMPLE 2 - LIMITED DETAILS MODAL
EXAMPLE 3 - FULL DETAILS MODAL