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

spike: style poc improvements #864

Merged
merged 2 commits into from
Jan 7, 2025
Merged

spike: style poc improvements #864

merged 2 commits into from
Jan 7, 2025

Conversation

Alessandro100
Copy link
Contributor

@Alessandro100 Alessandro100 commented Dec 20, 2024

closes #844
Summary:

Dives into best styling practices going forward. Implements these styling practices in a few pages to demonstrate the use case for each styling method.

Styling Methods

Note

This information will be stored in notion

Important

React does not have strict styling standard, and as such we have a lot of freedom and liberty 🦅 . There is no strict ‘right’ way, a lot will come down to context dependent, these styling methods are strong guidelines

Use MUI Components

The number 1 default should be to use components that exist in the MUI library. These are components that are well tests and cover a fair amount of use cases

Modifying the Theme.ts

When we want to apply a style to the entire application modifying the theme is the way to go ex: Button font

Using inline Sx styling

Should be used for 1 off styling and small style adjustments.

[good]

<Typography sx={{mt:1, px: 2}}>

[not ideal]

<Box sx={{
display: 'flex',
alignItems: 'center',
position: 'sticky',
zIndex: 1,
top: '65px',
background: theme.palette.background.default,
transition: 'box-shadow 0.3s ease-in-out',
mx: '-24px',
}}

Complex Sx Styles -> Separate File

Should be added into a separate file closely tied with the component ex: Feeds.style.ts

  • Naming convention: x.style.ts
  • .ts and not .tsx for clarity of content, these should not have any UI content in them
    These style files should hold variables that return Sx styles or styled components.

Note

If there are styles that could be reusable, they should be added in the global styles folder

Important

When importing a sx variable it can be particular. When using this prop alone you do not need to pass the theme
ex:

sx={verificationBadgeStyle}

When using it with other styles it's important to pass the theme manually (if applicable)
ex:

sx={(theme) => ({
display: 'block',
ml: 1,
...verificationBadgeStyle(theme),
})}

It's important to declare type as SystemStyleObject and not SxProp
ex:

export const verificationBadgeStyle = (
  theme: Theme,
): SystemStyleObject<Theme> => ({
  background: `linear-gradient(25deg, ${theme.palette.primary.light}, ${theme.palette.primary.dark})`,
  color: 'white',
});

Styled Components

Should be used when we want a specific component to have a consistent style throughout the application, but not default styled like it would be done through the Theme
ex: The main page header styling and containers with coloured backgrounds

export const ColoredContainer = styled(Container)(({ theme }) => ({

Warning

When using styled components, it currently does not accept Sx shorthand styling elements such as mx: 2. The compile wont throw an error, the browser will just add the css element mx: 2 which is invalid. There is an experimental feature from MUI called unstable_sx which allows this. It seems like there will be a future version that will support this

CSS files / modules

This method is not recommended, as the way we have our theme setup, we would not be able to easily access values from our Theme (ex: getting the primary colour / spacing). Although not recommended, it does have it's uses for complex css see web-app/src/app/styles/TextShimmer.css

Why All of This is Important

  • Having styling standards will make our web-app more consistent in appearance. It will promote re-usability
  • It will also allow for much quicker style edits that apply throughout the application
  • It will declutter the .tsx making the code more readable. ex: 10 Sx properties vs chipHolderStyles

Extra Notes

  • We don't need to add CssBaseline everywhere, just at the root should do it
  • We should be conscious of the HTML elements we use. We have a lot of Grid elements, that are not needed
  • Would be nice to update to MUI v6 but the MUI data table does not have support for it just yet
  • Performance: Using React MUI elements does take a slight performance hit, but it is very negligible and not noticeable in 99% of cases. It is something to consider if / when we hit a performance issue when rending a lot of elements
    image

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Run the unit tests with ./scripts/api-tests.sh to make sure you didn't break anything
  • Add or update any needed documentation to the repo
  • Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
  • Linked all relevant issues
  • [] Include screenshot(s) showing how this pull request works and fixes the issue(s)

@Alessandro100 Alessandro100 self-assigned this Dec 20, 2024
Comment on lines +26 to +29
export const stickyHeaderStyles = (props: {
theme: Theme;
isSticky: boolean;
}): SxProps => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great example of a refactored style that also handles dynamic styling

Comment on lines +3 to +11
export const MainPageHeader = styled(Typography)<TypographyProps>({
fontWeight: 700,
});

MainPageHeader.defaultProps = {
variant: 'h4',
color: 'primary',
component: 'h1',
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good example of how a styled component could also set default parameters

Copy link

Preview Firebase Hosting URL: https://mobility-feeds-dev--pr-864-7pjyth8v.web.app

@Alessandro100 Alessandro100 force-pushed the spike/web-app-styling branch from f23f2a0 to eda7c06 Compare January 6, 2025 14:00
Copy link

github-actions bot commented Jan 6, 2025

Preview Firebase Hosting URL: https://mobility-feeds-dev--pr-864-1uogmlj6.web.app

Copy link
Member

@davidgamez davidgamez left a comment

Choose a reason for hiding this comment

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

LGTM!

@Alessandro100 Alessandro100 merged commit b4d6274 into main Jan 7, 2025
4 checks passed
@Alessandro100 Alessandro100 deleted the spike/web-app-styling branch January 7, 2025 18:19
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.

Spike: Feeds page refactoring + styling structure
2 participants