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

Feature/add about page #212

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Feature/add about page #212

wants to merge 7 commits into from

Conversation

pthan1
Copy link
Collaborator

@pthan1 pthan1 commented Oct 22, 2021

Description

  • Link the footer to the About page
  • Implement the About.js component

Fixes #189

Type of Change:

  • Code
  • User Interface

Code/Quality Assurance Only

  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials

Code/Quality Assurance Only

  • My changes generate no new warnings
  • My PR currently breaks something (fix or feature that would cause existing functionality to not work as expected)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been published in downstream modules

@pthan1 pthan1 requested a review from galbwe October 22, 2021 15:12
@galbwe
Copy link
Collaborator

galbwe commented Oct 23, 2021

@pthan1 just a heads up, the main header and sub header render on top of each other when I view this

Screen Shot 2021-10-22 at 10 01 28 PM

@pthan1
Copy link
Collaborator Author

pthan1 commented Oct 23, 2021 via email

Copy link
Collaborator

@galbwe galbwe left a comment

Choose a reason for hiding this comment

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

Please run yarn format and yarn lint and make any requested changes.


export const useStyles = makeStyles((theme) => ({
// TODO: make custom roundButton component
roundButton: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you run yarn format from the frontend directory? That will make the formatting consistent with the main branch and reduce the number of diffs.

While you are there, also run yarn lint and make any changes the linter suggests. That should get the linter check in github to pass.

Comment on lines 226 to 228
<Link to="/about">
<Button className={classes.aboutFooter}>About</Button>
</Link>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the <Link /> component is adding some funky styling to the <Button /> text.

Screen Shot 2021-10-24 at 10 46 11 AM

One way around it is to remove the <Link /> component and use the useHistory hook to reroute. There might be a better way that explicitly styles the link, so I'm open to other approaches.

Suggested change
<Link to="/about">
<Button className={classes.aboutFooter}>About</Button>
</Link>
<Button className={classes.aboutFooter} onClick={() => history.push('/about')}>
About
</Button>

Copy link
Collaborator Author

@pthan1 pthan1 Oct 31, 2021

Choose a reason for hiding this comment

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

I've had luck switching the <Link /> to a <NavLink className={classes.link} />. What do you think?

const classes = useStyles();

return (
<Box className={classes.aboutPage}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to have a link back to /home from /about.

@pthan1 pthan1 requested a review from galbwe November 1, 2021 05:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add About Page
2 participants