joshwcomeau / guppy

🐠A friendly application manager and task runner for React.js
ISC License
3.27k stars 154 forks source link

Upgrade to styled-components v4.2.0 #374

Open superhawk610 opened 5 years ago

superhawk610 commented 5 years ago

See #269 for previous discussion.

Changes

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

superhawk610 commented 5 years ago

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.

codecov[bot] commented 5 years ago

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% <ø> (ø) :arrow_up:
...omponents/WhimsicalInstaller/WhimsicalInstaller.js 3.15% <0%> (ø) :arrow_up:
src/components/TerminalOutput/TerminalOutput.js 28% <0%> (ø) :arrow_up:
src/components/OnlineChecker/OnlineChecker.js 0% <0%> (ø) :arrow_up:
src/components/GlobalStyles/GlobalStyles.js 0% <0%> (ø)
src/components/Planet/PlanetMoon.js 26.31% <0%> (ø) :arrow_up:
src/global-styles.js 0% <0%> (ø) :arrow_up:
src/index.js 0% <0%> (ø) :arrow_up:
src/components/Planet/Orbit.js 9.09% <0%> (ø) :arrow_up:
src/components/LoadingScreen/LoadingScreen.js 0% <0%> (ø) :arrow_up:
... and 23 more
superhawk610 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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.

AWolf81 commented 5 years ago

@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.