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

Sk pja react router dom #25

Merged
merged 19 commits into from
Apr 13, 2020
Merged

Sk pja react router dom #25

merged 19 commits into from
Apr 13, 2020

Conversation

SaraSweetie
Copy link
Contributor

@SaraSweetie SaraSweetie commented Apr 10, 2020

Description

Utilizing react-router-dom for managing the page views. This app currently has two page views: the default shopping list, and /add to add an item to the shopping list. The currently selected page is indicated by a change in the button color and the browser's URL.

Related Issue

closes #4

Acceptance Criteria

  • react-router-dom has been added as a project dependency
  • 2 tabs are present and persistent at the bottom of the app: one for the “list” view, the other for the “add an item” view
  • Whichever tab is selected should indicate somehow that it is selected
  • When a link is clicked, the browser URL updates to represent the current view

Type of Changes

Type
🐛 Bug fix
✨ New feature
🔨 Refactoring
💯 Add tests
🔗 Update dependencies
📜 Docs

Updates

Before

Starter UI from create-react-app. This code was removed.

After

collab-tcl6-issue4

  • Two page views. Shopping List is the default /, and Add Item /add
  • The active page is highlighted in the UI and indicated in the URL.

Testing Steps / QA Criteria

  1. From your terminal, pull down the branch by using git pull origin sk-pja-react-router-dom and git checkout sk-pja-react-router-dom to switch to the branch.
  2. Type npm install to add the dependencies, then npm start to run the app.
  3. Use the buttons to switch the page view.
  4. The button for the currently viewed page will be highlighted and indicated in the URL. The default page is Shopping List, and Add Item will be indicated with /add in the URL.

src/App.js Outdated
@@ -1,25 +1,20 @@
import React from 'react';
import logo from './logo.svg';
import { BrowserRouter as Router, Switch, Route, Link } from 'react-router-dom';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think react-router-dom should be installed in this branch rather than having the reviewer install it. That might be why the checks are failing. Netlify doesn't know to install it unless it's already been added to the package.json.

Copy link
Contributor Author

@SaraSweetie SaraSweetie Apr 10, 2020

Choose a reason for hiding this comment

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

@skillitzimberg Thanks! This was very helpful and I realized what I did wrong. The package.json files are now updated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you npm install react-router-dom @SaraSweetie to install the library?

Copy link
Contributor Author

@SaraSweetie SaraSweetie Apr 11, 2020

Choose a reason for hiding this comment

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

@stevelikesmusic Yes. That is how I installed the react-router-dom library.

@skillitzimberg
Copy link
Contributor

Based on how I read the AC I thought that your PR had met the requirements. But I think that I'm not interpreting the last criterium the way it was intended.

When a link is clicked, the browser URL updates to represent the current view.

It might be a good idea to check in with mentors to clear up any confusion around this.

@segdeha segdeha requested a review from espain16 April 12, 2020 02:32
Copy link
Contributor

@skillitzimberg skillitzimberg left a comment

Choose a reason for hiding this comment

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

Looks good! There are a couple of minor clean up details. I left comments on/near the affected lines.

src/App.js Outdated Show resolved Hide resolved
Copy link
Contributor

@espain16 espain16 left a comment

Choose a reason for hiding this comment

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

There is an unused border property in your class selector called 'selected'.

src/App.css Outdated Show resolved Hide resolved
@SaraSweetie SaraSweetie marked this pull request as ready for review April 12, 2020 16:24
Copy link
Contributor

@skillitzimberg skillitzimberg left a comment

Choose a reason for hiding this comment

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

Looks good. Approved.

@erostribe erostribe merged commit c3ae2c0 into master Apr 13, 2020
@segdeha segdeha deleted the sk-pja-react-router-dom branch April 19, 2020 23:28
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.

2. As a user, I want to be able to navigate between the “list” view and the “add an item” view
5 participants