-
Notifications
You must be signed in to change notification settings - Fork 22
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
Karla 372 login button #1010
base: master
Are you sure you want to change the base?
Karla 372 login button #1010
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.