the-collab-lab / tcl-36-smart-shopping-list

1 stars 2 forks source link

fea: issue 2 add item and list view routes built plus navigation links #16

Closed alenamedved closed 2 years ago

alenamedved commented 2 years ago

For an example of how to fill this template out, see this Pull Request.

Description

Related Issue

closes #2

Acceptance Criteria

Type of Changes

Type
:bug: Bug fix
:sparkles: New feature
:hammer: Refactoring
:100: Add tests
:link: Update dependencies
:scroll: Docs

Updates

Before

before

After

after AddItemPage ListView

Testing Steps / QA Criteria

run npm install or npm i to add react-router to dependencies

rudidev08 commented 2 years ago

@alenamedved @shelleymcq I love that react router v6 upgrade was noted in comments. Since it's significant change, I'd have that as a top level bullet point with something like "using react router v6 instead of default vX. Notable change: (insert whatever seems to be huge change, if any, that you'd want coworkers to know). See https://dev.to/arunavamodak/react-router-v5-vs-v6-dp0 for more details". I'd even start that bullet item with "Note" that's bold or different color to draw attention, since it's significant change compared to other projects and it's not missed.

Current code (IMHO) is clean and obvious and doesn't need comments; for future definitely keep thinking as you're coding if you in 6 months from now, or with wiped memory, or coworker who has never seen the code could use comments. App will start to get more complex soon.

lindseyindev commented 2 years ago

We might need some help on this part.. I also had merge conflicts when pulling down from the package-lock.json and I think I used git pull --force to resolve it not sure how it would be resolved on github

conflicts

alenamedved commented 2 years ago

Wow - the bullet points explaining the changes in react-router-dom v6 were great! Very easy to follow and understand!

The only thing from the ACs that I think could be improved is this part:

Links are present and persistent at the bottom of the app

The navigation component lives underneath the other components, but my interpretation of the AC is that it stays towards the bottom of the page when the url changes. It could be interpreted other ways, but I think it makes sense that the nav bar would remain in a specific spot on the page even when the other components/pages develop and change sizes.

Very clear and clean PR - great job!

Hi @laurenyz Thank you for the review. I agree with you, the part about navbar can be improved. I think we didn't consider it was important cause we were going to add css later and fix this imperfection. But for now to make it look good and follow AC 100% I've added css and made navbar stick to the bottom.

laurenyz commented 2 years ago

Looks great, @alenamedved !