Skip to content
This repository has been archived by the owner on Jun 17, 2021. It is now read-only.

Upgrade to styled-components v4.2.0 #374

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Upgrade to styled-components v4.2.0 #374

wants to merge 18 commits into from

Conversation

superhawk610
Copy link
Collaborator

@superhawk610 superhawk610 commented Mar 28, 2019

See #269 for previous discussion.

Changes

  • upgrade styled-components to latest version (4.2.0)
    • change injectGlobal to createGlobalStyle
    • change withComponent at declaration to as when instantiating component
    • hoist props => values to top level in styled.el.attrs calls (see styled-components#2200)
    • change SC.extend to styled(SC)
  • upgrade flow-bin to latest version (0.95.1) to get forwardRef support

Known issues

bundle.js:112123 Warning: Stateless function components cannot be given refs. Attempts to access this ref will fail.
    in Icon
    in StyledComponent
    in AnimatedComponent (at FeedbackButton.js:97)
    in div
    in StyledComponent
    in Spring (at FeedbackButton.js:49)
    in FeedbackButton (at App.js:50)
    in Initialization (created by Connect(Initialization))
    in Connect(Initialization) (at App.js:33)
    in App (created by Connect(App))
    in Connect(App) (at index.js:26)
    in NodeProvider (at index.js:23)
    in Provider (at index.js:22)

Wrapping a styled component with animated()from react-spring seems to be the culprit for this error message, though from researching this briefly it appears to be harmless error (the ref is never used).

@idoberko2 you worked on the migration to react-spring, perhaps you can shed some light here?

Todo

  • update existing snapshot tests to include styled(Component) and <ForwardRef /> HOCs

@superhawk610
Copy link
Collaborator Author

Looks like this change also breaks all existing snapshot tests that relied on a styled component, as well as any that used a <TextInput /> as it's now wrapped with a <ForwardRef /> HOC. These will need to be updated.

@@ -251,11 +251,11 @@ export class Sidebar extends PureComponent<Props, State> {
}
}

const Wrapper = animated(styled.nav.attrs(props => ({
style: {
const Wrapper = animated(styled.nav.attrs({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ups, merging master changed this to old styled-component syntax - I'll fix this.

@codecov
Copy link

codecov bot commented Apr 24, 2019

Codecov Report

Merging #374 into master will decrease coverage by 0.03%.
The diff coverage is 64.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #374      +/-   ##
==========================================
- Coverage   58.83%   58.79%   -0.04%     
==========================================
  Files         158      159       +1     
  Lines        3357     3354       -3     
  Branches      467      468       +1     
==========================================
- Hits         1975     1972       -3     
  Misses       1179     1179              
  Partials      203      203
Impacted Files Coverage Δ
...nents/ProjectIconSelection/ProjectIconSelection.js 50% <ø> (ø) ⬆️
...omponents/WhimsicalInstaller/WhimsicalInstaller.js 3.15% <0%> (ø) ⬆️
src/components/TerminalOutput/TerminalOutput.js 28% <0%> (ø) ⬆️
src/components/OnlineChecker/OnlineChecker.js 0% <0%> (ø) ⬆️
src/components/GlobalStyles/GlobalStyles.js 0% <0%> (ø)
src/components/Planet/PlanetMoon.js 26.31% <0%> (ø) ⬆️
src/global-styles.js 0% <0%> (ø) ⬆️
src/index.js 0% <0%> (ø) ⬆️
src/components/Planet/Orbit.js 9.09% <0%> (ø) ⬆️
src/components/LoadingScreen/LoadingScreen.js 0% <0%> (ø) ⬆️
... and 23 more

Copy link
Collaborator

@AWolf81 AWolf81 left a comment

Choose a reason for hiding this comment

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

LGTM.

I've updated the snapshots & merged master.

The warning you mentioned is because IconBase is a functional component src of IconBase and React-Spring is passing a ref to it as mentioned in this issue. Not sure how to fix this - the wrapping in a class component will work but feels a bit hacky.

I commented one assertion in Sidebar.test. Not sure how to fix - we can fix this later. SetTimeout seems to be called 7 times but it should be only called twice.

One point I've noticed - Icon selection in ProjectSettingsModal is not working correctly. The citrone icon should be marked in the screenshot below.
grafik

@superhawk610
Copy link
Collaborator Author

Awesome, thanks for taking this up. I'll try to look into the icon issue if I get the time. I believe Josh had originally noticed some performance issues when we first looked at moving to v4, did you find there to be any performance degradation?

@AWolf81
Copy link
Collaborator

AWolf81 commented Apr 25, 2019

@superhawk610 after having a second look at the modal animation there is an issue with the project settings modal - there is no in animation & the modal is not reaching the final position and out animation is not smooth. I'll have a look if this is the same thing Josh noticed.
The CreateNewProject modal animation is working perfectly. Please have a look at the following screen recording.
Screenrecording_Modal_animations

Update
After doing some testing I have found the cause for the animation issue. In Modal.js it seems like there is a problem with interpolation for native prop. After testwise commenting native in line 106 it's animating correctly.
Not sure what's the problem - probably there is an interpolation required.

@AWolf81
Copy link
Collaborator

AWolf81 commented Apr 25, 2019

OK, I've fixed the forwardRef warning by creating a wrapper around the icon instead of directly using the animation on the icon.

Selection issue is also fixed - we checked for equality but src contained an initial dot in path on load. e.g. selectedIcon="/static/media/icon_fish6.9287e724.jpg" src="./static/media/icon_fish6.9287e724.jpg"
I first tried to remove the dot during the check with .slice(1) but that wasn't working because selecting an new icon didn't work with this.
I think testing with includes is OK.

I've also improved the animation so it's correctly rendering again but there is still a lot going on but I'm not seeing if there is anything wrong.

grafik
In the above screenshot it starts with a click event to open the modal and then at the end it's the closing click event.
The data from the above screenshot are available in this Gist - should be possible to load into Chrome Devtools.

Copy link
Collaborator

@melanieseltzer melanieseltzer left a comment

Choose a reason for hiding this comment

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

I'm noticing a regression with animation in the New Project view 😞 The Project Name box is supposed to animate down when you launch the modal, but in this branch it's stuck in its top position unless you start entering a project name (either by typing or clicking generate):

@AWolf81
Copy link
Collaborator

AWolf81 commented May 5, 2019

@melanieseltzer OK, I've fixed both mentioned issues.

To the project name animation
I think there is just an animation during modal opening but I'm not sure. The stucking happend because of a missing interpolation with native prop.

To the project selection issue
I've changed the undefined check a bit so flow check is working and displaying is working as expected.

Copy link
Collaborator

@melanieseltzer melanieseltzer left a comment

Choose a reason for hiding this comment

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

Icons look great! Animation is still not working right but I think I have a solution...

src/components/CreateNewProjectWizard/MainPane.js Outdated Show resolved Hide resolved
style: {
transform: `translateY(${props.translateY}px)`,
transform: interpolate([translateY], y => `translateY(${y}px)`),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately this interpolate change isn't producing quite the right result... it's still sticking to the top and not having the correct offset animation. The correct behavior has offset at 50 at first, and when moving to next step, offset becomes 0 and content shifts up smoothly:

But I dug into it a little more and I think I found the solution! Remove the native prop on Spring:

<Spring
  from={{
    offset: currentStepIndex === 0 ? 0 : 50,
  }}
  to={{
    offset: currentStepIndex === 0 ? 50 : 0,
  }}
  // remove
  native
>

And then reset this to the original code:

const Wrapper = animated(styled.div.attrs(({ translateY }) => ({
  style: {
    transform: `translateY(${translateY}px)`,
  },
}))`
  height: 75vh;
  will-change: transform;
`);

This fixes it for me :) I'm not really sure what native is or why it's here, do you know?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, OK. The native prop is there for better performance and we should keep it. docs about native prop.

Maybe inlining the translate into a style prop in MainPane could help. I'll have a look later today.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@melanieseltzer Please have a look at the animation. It should be OK now. I'm not sure why animation(styled.div) is not working - maybe I'll create a minimal demo in a Codesandbox to see if it's happening there too.
Could be an issue with Styled-components or React-spring because animation wrapping is recommended in React-Spring docs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've created a Codesandbox with it - seems like the style is not added on the element but I'm not sure why.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, the updated code works 👍 Not sure why it's doing this either. I noticed some more animations not firing so I went and updated them using the way you've outlined, could you double check the logic? Feel free to modify if there's a more elegant way to write these.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants