Karla 372 login button #1010
Karla 372 login button #1010
Conversation
There was a problem hiding this comment.
Thanks for submitting this! I gave you a few pointers on a few things to look at. I agree with your code comment about moving the logic for checking if someone has been auth'd already somewhere else, and I think putting it in App.jsx would make the most sense.
| @@ -3,7 +3,7 @@ import PropTypes from 'prop-types'; | |||
| import { Link, withRouter } from 'react-router-dom'; | |||
| import qs from 'qs'; | |||
| import { images } from 'assets'; | |||
| import styles from './Navigation.scss'; | |||
| import styles from './Navigation.module.scss'; | |||
There was a problem hiding this comment.
This change looks a little bit odd. You might not have branched from the latest master branch, where I had renamed all of the CSS module files to .module.scss. You probably do want this change, but it needs to go with all of the other corresponding changes, or else it won't work.
Could you merge the latest master branch into your branch to get this all up to date?
| /* get cookie value by key 'username', should this be inside renderAuthButton? */ | ||
| let cookieValue = document.cookie.replace(/(?:(?:^|.;\s)username\s*\=\s*([^;]*).*$)|^.*$/, "$1"); |
There was a problem hiding this comment.
I'd suggest pulling all this out into a higher level of the app so that it can be accessed by multiple different components on the site. We could probably create a React Context, which allows us to set some values at a higher level and access them at level levels without having to pass the values individually through the props of each component along the way.
I'd suggest creating the context in App.jsx, which sits above almost every page and component on the site, including the Navigation.
Also, separately, I'd suggest using a library like js-cookie to make the cookie handling nicer. I honestly have no idea how we ended up with such a crappy API for access cookies on the web.
| @@ -86,6 +98,8 @@ class Navigation extends React.Component { | |||
| Contact Us | |||
| </a> | |||
| </li> | |||
| {/* ADDED */} | |||
| {renderAuthButton()} | |||
There was a problem hiding this comment.
In React, you can actually have this act as a component be writing it as:
| {renderAuthButton()} | |
| <renderAuthButton /> |
React components are just special classes or functions, and function-based React components are just regular functions that accept props as an argument (or no argument) and return some JSX. Your renderAuthButton() function actually meets the criteria. I would, however, suggest that when you use a function as a React component, you give it a noun name rather than a verb name like "render". In this case, I think AuthButton would be a pretty good name for the component.
| /* get cookie value by key 'username', should this be inside renderAuthButton? */ | ||
| let cookieValue = document.cookie.replace(/(?:(?:^|.;\s)username\s*\=\s*([^;]*).*$)|^.*$/, "$1"); | ||
| if(!cookieValue){ | ||
| return <li> |
There was a problem hiding this comment.
Nit: your indentation here doesn't match normal indentation standards, where you'd indent the body of if statements and whatnot. Our linter should be able to automatically fix this and many other kinds of code formatting issues if you run npm run lint:fix.
No description provided.