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

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

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

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

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
1 participant