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.

Sign up for GitHub

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

Jump to bottom

Styled Components 5 #1543

Merged
9 commits merged into from Nov 16, 2020
Merged

Styled Components 5 #1543

9 commits merged into from Nov 16, 2020

Conversation

ghost
Copy link

@ghost ghost commented Oct 11, 2020 •
edited by ghost

✨ Changes

  • Upgrade to Styled Components 5
  • Upgrade to Typescript 3.9.7
  • Piles of snapshot upgrades

✅ Requirements

  • Includes test coverage for all changes
  • Build and tests are passing
  • Update documentation
  • Updated CHANGELOG
  • Checked for i18n impacts
  • Checked for a11y impacts
  • Check for image-snapshot changes (run yarn image-snapshots locally)
  • PR is ideally < ~400LOC

📷 Screenshots

@ghost ghost changed the base branch from master to luke/sc5-prep October 11, 2020 21:40
@ghost ghost force-pushed the luke/sc5-prep branch 2 times, most recently from 95c5651 to b37d81d Compare October 11, 2020 21:47
@ghost ghost force-pushed the luke/sc5 branch 2 times, most recently from 42d9555 to 72abdc0 Compare October 11, 2020 23:45
Base automatically changed from luke/sc5-prep to master October 12, 2020 05:45
@ghost ghost force-pushed the luke/sc5 branch 5 times, most recently from 01d7540 to 3dbbc18 Compare October 18, 2020 10:08
@ghost ghost force-pushed the luke/sc5 branch 2 times, most recently from d9929ab to 0827902 Compare October 26, 2020 04:08
@ghost ghost marked this pull request as ready for review November 16, 2020 04:12
ghost
ghost commented Nov 16, 2020
View reviewed changes
packages/components/src/DataTable/stories/State.tsx
@@ -33,7 +33,7 @@ import { DataTable, DataTableProps } from '../DataTable'
import { columns } from '../../__mocks__/DataTable/columns'

const Template: Story<DataTableProps> = ({ ...args }) => (
<DataTable columns={[]} {...args}>
<DataTable {...args}>
Copy link
Author

Choose a reason for hiding this comment

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

TS 3.9 caught this bug (columns specified twice)

ghost
ghost commented Nov 16, 2020
View reviewed changes
babel.config.js
Comment on lines +42 to +43
'**/*.story.*',
'**/stories/*',
Copy link
Author

Choose a reason for hiding this comment

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

Stories should not end-up in the build artifact.

package.json
@@ -72,7 +72,7 @@
"jest": "^26.5.3",
"jest-canvas-mock": "^2.3.0",
"jest-image-snapshot": "^4.2.0",
"jest-styled-components": "^6.3.4",
"jest-styled-components": "^7.0.3",
Copy link
Author

Choose a reason for hiding this comment

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

jest-styled-components 7.x is compatible with styled-components 5.x

packages/components/src/Dialog/Layout/DialogContent.tsx
@@ -68,7 +68,7 @@ const DialogContentLayout: FC<DialogContentLayoutProps> = ({

return (
<div
className={`${className} ${overflow ? 'overflow' : ''}`}
className={overflow ? `overflow ${className}` : className}
Copy link
Author

Choose a reason for hiding this comment

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

Previously was appending extra whitespace for no reason

packages/components/src/List/List.test.tsx
expect(list).toBeInTheDocument()
expect(list).toHaveStyleRule('list-style-type', 'none')
expect(list).toHaveStyle('list-style-type: none;')
Copy link
Author

Choose a reason for hiding this comment

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

toHaveStyleRule wasn't getting picked up properly in SC5. Other clean-ups were just included since I was refactoring anyway.

ghost
ghost commented Nov 16, 2020
View reviewed changes
packages/components/src/Form/Inputs/Radio/__snapshots__/Radio.test.tsx.snap
Comment on lines -277 to -303
.c3 input:checked + .c1 {
color: #6C43E0;
}

.c3 input:not(:checked) + .c1 {
background: #FFFFFF;
border-color: #DEE1E5;
}

.c3 input:focus + .c1 {
border-color: #9785F2;
box-shadow: 0 0 0 2px #E8E5FF;
outline: none;
}

.c3 input:disabled + .c1 {
color: #939BA5;
}

.c3 input:disabled:not(:checked) + .c1 {
background: #F5F6F7;
}

.c3 input:disabled:not(:checked) + .c1::after {
background: #F5F6F7;
}

Copy link
Author

Choose a reason for hiding this comment

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

Not sure why this isn't emitted anymore but Components still works so 🤷

I think SC5 might be smart enough to emit the pseudo styles more cleverly now.

packages/components/src/Form/Inputs/TextArea/__snapshots__/TextArea.test.tsx.snap
Comment on lines -329 to -340
.c3 .c1 {
position: absolute;
right: 0.5rem;
top: 0.5rem;
}

.c4 .c1 {
position: absolute;
right: 0.5rem;
top: 0.5rem;
}

Copy link
Author

Choose a reason for hiding this comment

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

This is used to position alert. Still works even with this snapshot change. Again, I think SC is "tree shaking" unused CSS better now.

@ghost ghost requested a review from mdodgelooker November 16, 2020 16:58
@ghost
Copy link
Author

ghost commented Nov 16, 2020

@mdodgelooker I'd like to land this today so I can work on the other side of the house. You'll notice some changes to snapshots where some CSS isn't emitted anymore - I'm pretty sure SC5 introduces a better "tree shaking" behavior where it doesn't emit unused CSS in cases where it's not needed.

mdodgelooker
mdodgelooker reviewed Nov 16, 2020
View reviewed changes
packages/components/src/Avatar/__snapshots__/Avatar.test.tsx.snap
@@ -145,11 +145,11 @@ exports[`Avatar AvatarCombo renders Avatar and its secondary avatar 1`] = `
/>
</div>
<div
className="c4 c5"
className="AvatarCombo__AvatarIconSecondary-gekgjm-0 c4"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it seem like something's not quite working with jest-styled-components when there's nested styled components? AvatarCombo__AvatarIconSecondary-gekgjm-0 should still be c4, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

styled-components/jest-styled-components#276

Luke Bowerman added 7 commits November 16, 2020 19:12
Styled Components 5 prep Test fix-ups
a27ae7a 
Refactored DialogContent to make remove class component, removed
problematic test and replaced with Storybook image-snapshot

Remove `FieldText.stories` storybook - it wasn't useful

Replaced instances of `toHaveStyle` with `toHaveStyleRule` (former
doesn't work well with SC5)
Styled Components 5 prep Test fix-ups
4e4e742 
Refactored DialogContent to make remove class component, removed
problematic test and replaced with Storybook image-snapshot

Remove `FieldText.stories` storybook - it wasn't useful

Replaced instances of `toHaveStyle` with `toHaveStyleRule` (former
doesn't work well with SC5)
Styled Components 5.x package updates
2a3bdb4 
Babel tweak and snapshot updates
Upgrade to TS 3.9.4
Snapshot updates
Refine Tooltip test (no longer needs runTimers)
f1e81d3
Fix build error
1afd996
Update DialogContent fixes
fbd60cc
Upgrade to latest, revert tooltip mistakes
7143650
mdodgelooker
mdodgelooker approved these changes Nov 16, 2020
View reviewed changes
Copy link
Contributor

@mdodgelooker mdodgelooker left a comment

Choose a reason for hiding this comment

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

Ship it!

packages/components/src/Layout/Box/__snapshots__/Box.test.tsx.snap
Comment on lines +157 to +160
@media screen and (min-width:30rem) {

}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is also mentioned in that same issue 🤷‍♂️
styled-components/jest-styled-components#276

@ghost ghost merged commit ff1c4f5 into master Nov 16, 2020
@ghost ghost deleted the luke/sc5 branch November 16, 2020 21:38
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
1 participant
@mdodgelooker