-
Notifications
You must be signed in to change notification settings - Fork 167
Add support for styled-components v6 #986
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
Conversation
ebcb247 to
9fba533
Compare
package.json
Outdated
| "fast-memoize": "^2.5.2", | ||
| "lodash.isequal": "^4.5.0", | ||
| "react-popper": "^2.3.0", | ||
| "stylis": "^4.3.6", |
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.
This is a peer dependency used by styled-components v6, a CSS preprocessor
| import { BaseSdkProps } from '../Base'; | ||
|
|
||
| const StyledPreview = styled(VideoTile)` | ||
| const StyledPreview = styled(VideoTile).withConfig(styledComponentsConfig)` |
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.
This is to handle a breaking change, in v5 the custom props such as isEnabledForScreenSharing is not forwarded to DOM element because it's not a native HTML attribute. So these non-native props are filtered. In v6 this filter is no longer provided by default, we follow the below guide to preserver the v5 behavior.
| & a:visited, | ||
| & a:hover, | ||
| & a:active { |
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.
This is to handle https://styled-components.com/docs/faqs#nested-syntax-handling
| ${isVertical(props.layout) ? layoutMap['left'] : layoutMap['bottom']}; | ||
| box-shadow: ${props.theme.controlBar.shadow}; | ||
| border: none; | ||
| height: ${(props: StyledControlBarProps) => | ||
| isVertical(props.layout) && '100%'}; | ||
| width: ${(props: StyledControlBarProps) => | ||
| !isVertical(props.layout) && '100%'}; | ||
| height: ${isVertical(props.layout) && '100%'}; | ||
| width: ${!isVertical(props.layout) && '100%'}; | ||
| } | ||
| ${({ theme }) => theme.mediaQueries.max.xs} { | ||
| justify-content: ${(props: StyledControlBarProps) => | ||
| isVertical(props.layout) ? 'center' : 'space-around'}; | ||
| justify-content: ${isVertical(props.layout) ? 'center' : 'space-around'}; | ||
| ${unsetPosition} | ||
| ${(props: StyledControlBarProps) => | ||
| isVertical(props.layout) ? layoutMap['left'] : layoutMap['bottom']}; | ||
| ${isVertical(props.layout) ? layoutMap['left'] : layoutMap['bottom']}; |
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.
This is to handle breaking TypeScript change, it becomes more strict.
| <GlobalStyles /> | ||
| <Canvas> | ||
| <Heading style={{ color: 'blue' }} level={3} tag="p"> | ||
| <Heading style={{ color: 'blue' }} level={3} tag="span"> |
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.
This is to handle a non-breaking error in storybook that a <p> can not be nested within another <p>
| theme.colors[severity].primary}; | ||
| color: ${({ theme, severity }) => | ||
| theme.notification[severity].closeButton.text}}; | ||
| theme.notification[severity].closeButton.text}; |
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.
Removed the extra }
| width: 100%; | ||
| height: 100%; | ||
| object-fit: ${(props) => props.objectFit || 'cover'}}; | ||
| object-fit: ${(props) => props.objectFit || 'cover'}; |
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.
Removed the extra }
| @@ -1,4 +1,5 @@ | |||
| import { WithTooltip } from '.' | |||
| import { Meta } from '@storybook/blocks'; | |||
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.
This page is broken now, this change fix it:
https://aws.github.io/amazon-chime-sdk-component-library-react/?path=/docs/ui-components-withtooltip--page
9fba533 to
5af0fdf
Compare
| "styled-components": "^5.3.9", | ||
| "styled-components": "^6.1.15", | ||
| "styled-system": "^5.1.5", | ||
| "stylis": "^4.3.6", |
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.
This is a CSS preprocessor, styled-components v6 requires this package
bc2a79a to
cf6daad
Compare
cf6daad to
4177aa4
Compare
4177aa4 to
e606e05
Compare
e606e05 to
a071c13
Compare
| // Forward custom props | ||
| if (additionalProps.includes(prop)) return true; | ||
|
|
||
| return isPropValid(prop); |
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.
For nested styled components, we add withConfig only to the bottom most one for the filtering. This way it won't block any react props being passing through a styled components.
For example:
const StyledVideoTile = styled.div.withConfig()`...`;
// React component
const VideoTile = (props) => {
return props.isActive? <StyledVideoTile/> : null
}
// Styled react component
const ContentTile = styled(VideoTile)`...`; // Do not use withConfig, because StyledVideoTile already have it, this way isActive will not be filtered.
<StyledContentTile isActive={true} />
a071c13 to
a07feae
Compare
| * @returns Configuration object with shouldForwardProp function | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/explicit-function-return-type | ||
| export const createStyledConfig = (additionalProps: 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.
no need to export this func?
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.
export is needed. The idea is that other components can use this function to forward custom props via additionalProps if needed. It's not being use at this moment.
Issue #:
Description of changes:
Testing
Have you successfully run
npm run build:releaselocally?Yes
How did you test these changes?
npm run build:releasepasses.I verified the following scenarios when installing styled-components v5 and v6 in the local React Component SDK:
When installing v6 in demo, both demo and SDK uses v6
When installing v5 in demo, both demo and SDK uses v5
Yes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.