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

Finished adding the rest of the content. Adjusted grid and styling. #1338

Merged
merged 2 commits into from
Sep 14, 2022

Conversation

funbunch
Copy link
Member

@funbunch funbunch commented Sep 6, 2022

Fixes #1298

  • Up to date with dev branch
  • Branch name follows guidelines
  • All PR Status checks are successful
  • Peer reviewed and approved

Any questions? See the getting started guide

@funbunch
Copy link
Member Author

funbunch commented Sep 6, 2022

Lastest change here:

Screen Shot 2022-09-06 at 1 09 57 PM

I still need to adjust mobile layout, but I will need to discuss changing the nav too which isn't responsive it looks like.

@funbunch funbunch requested a review from nichhk September 6, 2022 20:20
@nichhk nichhk requested review from edwinjue and removed request for nichhk September 8, 2022 03:09
@edwinjue
Copy link
Member

edwinjue commented Sep 9, 2022

@funbunch Thanks for providing me with the figma you used to base your changes on. Everything looks spot on. Besides a few minor things, the only feedback I have is just whether @nichhk would like us to inherit more styles from theme.js. If that is the case, we would likely make less use of makeStyles and utilize things like:

  • <Typography> components for things like h1, h2 and p styles which are defined here so the style definitions are defined in the central theme.js file

or optionally, within makeStyles, make use of the theme parameter to inherit styles from theme.js, for example:

const useStyles = makeStyles( (theme) => ({
  root: {
    color: theme.palette.text.primary,
    backgroundColor: theme.palette.background,

I will wait until @nichhk confirms this is the approach we are aiming for. And if there is a need for me to show how we could utilize this in future releases, I would be glad to demonstrate. Thanks for considering my perspective

client/components/main/About.jsx Outdated Show resolved Hide resolved
client/components/main/About.jsx Outdated Show resolved Hide resolved
client/components/main/About.jsx Outdated Show resolved Hide resolved
@@ -64,30 +65,70 @@ const About = () => {
the expansive amount of data associated with these 311
requests is available online. The mayor has encouraged
us to create apps with this data, and that&apos;s where
this project comes in.
this project comes&nbsp;in.
Copy link
Member

Choose a reason for hiding this comment

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

can all instances of &nbsp; be replaced with a single space?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what you mean? non breaking space here is to prevent the orphan text.

Copy link
Member

Choose a reason for hiding this comment

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

i see. the reason just wasn't clear. but if you find it important to keep those words together this will definitely work

Copy link
Member

@edwinjue edwinjue 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 and approved, thanks for considering my feedback!

@funbunch funbunch merged commit a4ec302 into dev Sep 14, 2022
@funbunch funbunch deleted the About branch September 14, 2022 21:33
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.

3 participants