-
Notifications
You must be signed in to change notification settings - Fork 85
feat: styled system replacement poc work - FE-5531 #7599
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
base: master
Are you sure you want to change the base?
Conversation
dd6f323 to
2f319d9
Compare
| ## Potential Names | ||
| - `carbon-style` - might be a bit vague. | ||
| - `styled` - sounds similar to `styled-system` and `styled-components` and might also imply a connection to those libraries. | ||
| - `Styley McStyleFace` - just to see who actually read this far. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: no further discussion needed IMO. This is the way.
|
|
||
| Component file: | ||
| ```tsx | ||
| import { paddingProps } from "../utils/spacing-types"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: PaddingProps to match casing in spacing-types
| export const Spacing: Story = () => { | ||
| return ( | ||
| <> | ||
| <DipsBox m="3px" p="3px"> | ||
| Margin and Padding String | ||
| </DipsBox> | ||
|
|
||
| <DipsBox m={3} p={3}> | ||
| Margin and Padding Number | ||
| </DipsBox> | ||
|
|
||
| <DipsBox ml={3} pt={3}> | ||
| Margin Left and Padding Top Number | ||
| </DipsBox> | ||
|
|
||
| <DipsBox ml="9px" pt="33px"> | ||
| Margin Left and Padding Top String | ||
| </DipsBox> | ||
|
|
||
| <DipsBox ml="XS" pt="L"> | ||
| Margin Left and Padding Top String | ||
| </DipsBox> | ||
| </> | ||
| ); | ||
| }; | ||
| Spacing.storyName = "Spacing"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: demonstrate that the types can mix-and-match e.g.:
<DipsBox m="3px" pt="L" pb={4}>There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a good idea. I'll get that added in the next round of changes.
| export interface MaxWidthProps { | ||
| maxWidth?: string; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question(minor): do we need this interface if maxWidth is part of LayoutProps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not, I did this because we have a MaxWidthProps that is imported from styled-system that is used in MenuItem and Submenu. I did this to take make sure my solution would work as a drop in replacement for styled-system.
| */ | ||
|
|
||
| // Mapping of size to multiplier value for spacing calculations | ||
| export const spacingSizeMap = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: could we defer these new size aliases to a second pass?
Since we didn't support them before, I think focusing on replacing the existing functionality from styled-system first would help minimise the number of changes and make testing for regressions easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we could. I mainly did this to prove to us and DS that this solution would support the sizing alias when we choose to implement them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah for me it would be a nice to have rather than need at this stage. For me the first pass must have a replacement for styled-stystem props that maintains the string and number (where applicable) interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I'll do for this then is make a copy of the branch in it's current form (with the sizing map) and keep it to one side for when/if we want to implement this. I've proven we can support it with the way the POC currently is, with no great deal of change required to the core spacing functions.
| // Remove any null values from paddingProps to ensure type compatibility | ||
| const paddingProps = Object.fromEntries( | ||
| Object.entries(paddingPropsRaw).filter( | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| ([_, paddingValue]) => paddingValue !== null, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment: if we can update spacing.js to be written in TypeScript, we might be able to remove these null checks, assuming TypeScript would be able to infer the values correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did consider using TS initially but I figured for a POC just using plain JS would be better. When we come to actually implement it, we could do it in TS. I don't think it would be too difficult to do.
| const props = { [styledProp]: 3 }; | ||
| render(component({ ...props })); | ||
|
|
||
| assertStyleMatch({ [propName]: "24px" }, elementQuery(), assertOpts); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: I noticed the default values of the new test utils differ from their old counterparts.
Keeping the old default values could help limit the amount of component code changes we'd need to make, as if we've replaced styled-system's functionality, we'd only need to swap the references to the old util with the new one:
| const props = { [styledProp]: 3 }; | |
| render(component({ ...props })); | |
| assertStyleMatch({ [propName]: "24px" }, elementQuery(), assertOpts); | |
| const props = { [styledProp]: 2 }; | |
| render(component({ ...props })); | |
| assertStyleMatch({ [propName]: "var(--spacing200)" }, elementQuery(), assertOpts); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment: would it be possible to revert these Menu changes?
I noticed if we update the testStyledPadding util to use the same default padding values as the old testStyledSystemPadding util, our tests still pass without these component changes. Keeping things scoped to the new styled-system replacement might make it easier for us to spot any behavioural differences.
Happy to discuss if I'm overlooking something :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean revert everything excluding the changes:
padding-right: ${(props) =>
parsePadding(props as Partial<PaddingProps>).padding};
And these:
right: ${(props) =>
parsePadding(props as Partial<PaddingProps>).iconSpacing};
And also keep the new imports?
|
|
||
| ## Implementation | ||
|
|
||
| The implementation involves creating utility functions that mimic the behavior of `styled-system` for common use cases, such as spacing, layout, and flexbox. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| The implementation involves creating utility functions that mimic the behavior of `styled-system` for common use cases, such as spacing, layout, and flexbox. | |
| The implementation involves creating utility functions that mimic the behaviour of `styled-system` for common use cases, such as spacing, layout, and flexbox. |
| */ | ||
|
|
||
| // Mapping of size to multiplier value for spacing calculations | ||
| export const spacingSizeMap = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah for me it would be a nice to have rather than need at this stage. For me the first pass must have a replacement for styled-stystem props that maintains the string and number (where applicable) interface
| const style = {}; | ||
| for (const key in flexboxKeys) { | ||
| const value = props[key]; | ||
| if (value == null) continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (value == null) continue; | |
| if (!value) continue; |
you could just check for any falsy value as it will only be null or ""
| * `${(props) => flexboxCss(props)}` | ||
| * in a styled component. | ||
| */ | ||
| // Number has been added to the logic here in case we choose to support numbers. For now flexGrow and flexShrink are string values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment: For me strings would be enough here, the only one I'd support numbers on is the spacing values and even on that I'm a bit on the fence as it's a bit of an opaque interface that requires documenting that the number is multiplied by a factor of 8 (mainly keeping them to lessen the impact of the breaking change)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strings alone would be easier to support, so I'm all for this. I did this number stuff as we have unit tests that test numbers for flexGrow and a few others. I can amend the data we pass in when I update the utils like @Parsium has requested earlier on in the PR.
| */ | ||
| // Number has been added to the logic here in case we choose to support numbers. For now flexGrow and flexShrink are string values | ||
| // but they are supposed to be numbers in CSS. | ||
| export function flexboxCss(props) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: unless we need the get... functions to be separate you could combine them so that the returned object is stringified rather than separating the concerns unnecessarily
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can mash them together no problem. For me it was easier to have them separated for debugging purposes, that was the main reason I did it this way.
| // 0 is special: "0" is valid everywhere | ||
| if (value === 0) return "0"; | ||
|
|
||
| // treat numbers in [0, 1] as percentages for percent-capable props |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment: imo this part of the interface was always terrible. Happy to go with the flow so if general consensus is to keep it to limit the impact of the breaking change then I can live with it. For me it's not very intuitive how to pass a float to achieve a percentage, for me a string suffices etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the idea behind this was to minimise the breaking change impact. It was also to demonstrate that if needs be, we can support with existing float stuff too if enough consumers shout about it. I'm happy to remove support for the percentage stuff and we can just document how to do percentages in the docs for the new approach.
| /** format a single value for output */ | ||
| function formatValue(cssKey, value) { | ||
| if (typeof value === "number") { | ||
| // If it's suffix-less return as raw number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: this has a suffix though?
| return value; | ||
| } | ||
|
|
||
| export function getSpacingStyles(props, scale = 8, sizeMap = spacingSizeMap) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment: same about combining this with the css one below
| */ | ||
|
|
||
| /** Apply the chosen scale to a numeric step/multiplier */ | ||
| function applyScale(n, scale) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment: I'd be pretty keen not to support this anymore in the interest of simplifying the implementation considerably. As far as I know this doesn't work in carbon anyway so I don't think removing it will have much impact. If I'm wrong about that then I still think it's worth discussing if there's any value persisting it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main idea for this was that DS have different spacing values based on component or layout styling. The idea being that layout might be a multiplier of 8 and component might be of 2. They were also keen to support passing token values as multipliers too. I'm more than happy to kick this can down the road for now though (it's been demonstrated here that we can add this functionality in to the core functions of this thing) and we just support what we currently support and implement this as a feature/improvement later on.
WIP POC. I am aware that we have a coverage issue, but this is due to the changes I have had to make in the menu-item styling. This is a proof of concept so my aim was to show that my proposal could work with a complex component.
Proposed behaviour
POC for replacing
styled-systemCurrent behaviour
styled-systemexistsChecklist
d.tsfile added or updated if requiredQA
Additional context
I have refrained from running the Chromatic workflow due to the not wanting to burn through Chromatic snapshots.
Testing instructions