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

Update front page to MUI components 1547 #1554

Conversation

freaky4wrld
Copy link
Member

@freaky4wrld freaky4wrld commented Dec 9, 2023

Fixes #1547

What changes did you make and why did you make them ?

  • Replaced <div> with <Box>
  • Created a theme and added styles
  • Replaced <h1>,<h2>,<h4> with Typography

Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)

Visuals before changes are applied

image

Visuals after changes are applied image

Copy link

github-actions bot commented Dec 9, 2023

Want to review this pull request? Take a look at this documentation for a step by step guide!

From your project repository, check out a new branch and test the changes.

git checkout -b freaky4wrld-update-front-page-to-MUI-components-1547 development
git pull https://github.com/freaky4wrld/VRMS.git update-front-page-to-MUI-components-1547

Copy link
Member

@trillium trillium left a comment

Choose a reason for hiding this comment

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

This solution to me looks like overkill. The project already has a provided theme (see /client/src/theme/index.js), I think we can add your additional CSS properties there if necessary rather than creating a whole new ThemeProvider.

@trillium
Copy link
Member

trillium commented Dec 14, 2023

I think you're on the right track when it comes to applying the font, but I'm not sure the changes are having the intended result:

Screenshot image

@freaky4wrld
Copy link
Member Author

This solution to me looks like overkill. The project already has a provided theme (see /client/src/theme/index.js), I think we can add your additional CSS properties there if necessary rather than creating a whole new ThemeProvider.

@spiteless on reviewing the page in browser it looks like the h1, h2 and h4 element are getting there styles from the file index.scss, the styles added by me in the ThemeProvider are same as in the file index.scss. Now I'm thinking to try and make those changes in the file /client/src/theme/index.js, do you think it'll work??

@freaky4wrld
Copy link
Member Author

freaky4wrld commented Dec 14, 2023

I think you're on the right track when it comes to applying the font, but I'm not sure the changes are having the intended result:

Screenshot

@spiteless some new changes are made, can you please review them. I guess the font and the alignment is achieved but if you remove the margin properties a big spacing issue is still there which is different from the original UX design.

@trillium
Copy link
Member

Looks like we don't need to modify the theme.js file at all actually. just need to pass the required css to the sx prop.

I have a working version locally but had to run off to work. Will provide some more guidance in a little bit.

@trillium
Copy link
Member

// Defined outside Home function
const h1sx = {
  fontFamily: 'aliseoregular',
  fontWeight: 'bold',
  fontSize: {xs: "5.3rem"},
}

const h2sx = {
  ...h1sx,
  fontSize: {xs: '2.8rem'},
}
<Box className="home-headers">
  <Typography sx={h1sx} variant='h1'>
    VRMS
  </Typography>
  <Typography sx={h2sx} variant='h2'>
    Volunteer Relationship Management System
  </Typography>
</Box>

Still needs some finagling on line height/padding, etc

Copy link
Member

@trillium trillium left a comment

Choose a reason for hiding this comment

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

Looking good! Please edit the properties slightly, particularly line height on h2 and font size on h1 as per below:

const h1sx = {
  fontFamily: 'aliseoregular',
  fontWeight: 'bold',
  fontSize: {xs: "6.0rem"},
  marginBottom: `0rem`,
}

const h2sx = {
  ...h1sx,
  fontSize: {xs: '2.8rem'},
  marginTop: '-0.9rem',
  lineHeight: '0.9',
}

const h4sx = {
  ...h1sx,
  fontSize: {xs: '1.8rem'},
}

@freaky4wrld
Copy link
Member Author

@spiteless I've tried to edit your working solution I guess it is pretty close, but a problem is arose after I updated the feature branch with the development branch, it shows an error for

/src/components/ChangesModal.js
[1] Module not found: Can't resolve '@mui/icons-material/WarningAmberRounded' 

maybe some recent commits to the development branch are causing this !! Can you please check this as well

@trillium
Copy link
Member

Heard on your error, please run yarn install again in the client folder

@trillium
Copy link
Member

@freaky4wrld No worries if you'd rather use rem units for lineHeight, that's a better move, but please take a look at the differences side by side:

image

Ultimately the exact numbers don't matter much, it'll get changes later on when a new version of the login page is made, but I'd prefer if the total size of the h1 and h2 together no taller than the current version.

Copy link
Member

@trillium trillium left a comment

Choose a reason for hiding this comment

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

Thanks! One page down :)

@trillium trillium merged commit e8f0f9c into hackforla:development Dec 16, 2023
5 checks passed
@freaky4wrld
Copy link
Member Author

@spiteless sorry for the naiveness, the error had me this time, so can't check on the changes it

Thanks! One page down :)

Thanks for helping throughout the issue 🥇

@freaky4wrld freaky4wrld deleted the update-front-page-to-MUI-components-1547 branch December 17, 2023 05:14
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.

Update Home.js Page page to MUI components
2 participants