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

frontend: Set theme spacing #3138

Merged
merged 2 commits into from
Oct 14, 2024
Merged

frontend: Set theme spacing #3138

merged 2 commits into from
Oct 14, 2024

Conversation

septum
Copy link
Contributor

@septum septum commented Oct 9, 2024

Description

  • Set the theme spacing
  • Create and export a ThemeSpacing enum

Testing Performed

Manual testing

@septum septum requested a review from a team as a code owner October 9, 2024 18:28
Comment on lines 49 to 51
Base,
Medium,
Large,
Copy link
Contributor

Choose a reason for hiding this comment

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

What do these represent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going for generic names for the spacing variants as using something like PageGutter could be self-descriptive, but the value could be repeated somewhere else.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do these map to something already? Curious why some are given a value while the others aren't. I haven't looked at the spacing variants for mui so maybe it's explained there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really a map, these values, which are in order 0, 0.5, 1, 2, 3 and 4, are used as a multiplier of the scaling factor to produce a value, so this enum is just a giving them a name.

It's expected to apply the enum members as following examples:

<Grid
- spacing={2}
+ spacing={ThemeSpacing.Base}
>
  <Component />
 </Grid>
const StyledComponent = styled(Component)(({ theme }: { theme: Theme }) => ({
- margin: "24px",
+ margin: theme.spacing(ThemeSpacing.Medium),
}));

There's an example in MUI's docs talking about the custom spacing.

frontend/packages/core/src/Theme/types.tsx Outdated Show resolved Hide resolved
@jecr
Copy link
Contributor

jecr commented Oct 9, 2024

Just to get a clearer idea, how would this be used in the field?, does this replace the values the current theme.spacing outputs?

@septum
Copy link
Contributor Author

septum commented Oct 9, 2024

Just to get a clearer idea, how would this be used in the field?, does this replace the values the current theme.spacing outputs?

Copy-pasting the example from above, it's expected to be used as the following examples:

<Grid
- spacing={2}
+ spacing={ThemeSpacing.Base}
>
  <Component />
 </Grid>
const StyledComponent = styled(Component)(({ theme }: { theme: Theme }) => ({
- margin: "24px",
+ margin: theme.spacing(ThemeSpacing.Medium),
}));

@@ -20,6 +20,9 @@ const createTheme = (variant: ThemeVariant): MuiTheme => {
return createMuiTheme({
colors: clutchColors(variant),
palette: palette(variant),
// `8` is the default scaling factor in MUI, we are setting it again to make it explicit
// https://v5.mui.com/material-ui/customization/spacing/
spacing: 8,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the usage example 🙌
So, just a thought, would it be a good idea to include the spacing options as properties of the theme object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going with that approach at first, but after discovering the theme.spacing method, I wanted to take advantage of what it could be done with it, like the multiple arity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, I think they can coexist, something like theme.spacing(theme.standardSpacing.medium) and in that standardSpacing you pretty much do the same you already have
I can see the naming and having to access the properties in theme twice a bit cumbersome 😅 though
What I do like about this is centralising everything in theme

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I think centralizing it would improve its DevEx.

Maybe we could add a new function to be used instead of spacing(), or come up with a different naming strategy for the spacing to make it concise?

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like theme.clutchSpacing.medium?, it is still long 😆 but yeah, that sounds easy to use

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the clutch prefix makes sense, so I turned that into an object in b40fc87. It ended up like:

theme.spacing(theme.clutch.spacing.md);

I was aiming to create something that only relies on the variant being the functions themselves, but that would require ugly workarounds, like creating the theme twice. Also, using this was unstable, even with arrow functions.

Should we settle for this?

cc @jdslaugh

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think this is a good way to go for the time being.

@septum septum merged commit d33408a into main Oct 14, 2024
8 checks passed
@septum septum deleted the theme-spacing branch October 14, 2024 21:41
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