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

Migration to react-spring and minor fixes #198

Merged
merged 10 commits into from Aug 26, 2018

Conversation

Projects
None yet
3 participants
@deamme
Copy link
Contributor

deamme commented Aug 20, 2018

Still work in progress..

Fixes #192

@deamme deamme force-pushed the deamme:master branch 2 times, most recently from ba67393 to d561e89 Aug 20, 2018

@sohkai sohkai requested a review from bpierre Aug 21, 2018

@deamme deamme force-pushed the deamme:master branch 2 times, most recently from 976ffce to 00df367 Aug 21, 2018

@deamme deamme force-pushed the deamme:master branch from 00df367 to 9f93bcb Aug 21, 2018

@bpierre bpierre removed their request for review Aug 21, 2018

@bpierre bpierre added the wip label Aug 21, 2018

@deamme

This comment has been minimized.

Copy link
Contributor

deamme commented Aug 21, 2018

Having trouble getting the last component right (CricleGraph). This line:
https://github.com/aragon/aragon-ui/blob/master/src/components/CircleGraph/CircleGraph.js#L40

I don't know how to replicate that with react-spring because you don't have access to the interpolated value in that case.

EDIT:
Same thing here:
https://github.com/aragon/aragon-ui/blob/master/src/components/DropDown/DropDown.js#L119

@sohkai sohkai requested a review from bpierre Aug 22, 2018

@bpierre
Copy link
Member

bpierre left a comment

Thanks for working on this, it’s looking good already

I added a few comments, let me know if you have any question about them. About DropDown, we can probably use onRest to update a value when the animation stops, rather than checking the progress.

style={{
openProgress: spring(Number(opened), springConf('fast')),
<Spring
config={springs.slow}

This comment has been minimized.

@bpierre

bpierre Aug 22, 2018

Member

We should probably use a faster transition here. NavigationBar is not ready to be merged yet, but we could copy the smooth spring from it: https://github.com/aragon/aragon-ui/pull/194/files#diff-095784126fa8bb5bdc7b1a53fc7c6f57

config={springs.lazy}
to={{
progressValue: value,
}}

This comment has been minimized.

@bpierre

bpierre Aug 22, 2018

Member

Coding style nitpick: we generally use a single line for single entry objects: to={{ progressValue: value }}.

strokeWidth: BORDER_WIDTH,
}}
/>
<Label x="50%" y="50%">
{label(Math.min(value, Math.max(0, progressValue)))}
{label(Math.min(value, Math.max(0, value)))}

This comment has been minimized.

@bpierre

bpierre Aug 22, 2018

Member

I asked the react-spring community if there is any recommended way to do this: https://spectrum.chat/react-spring?thread=5e147872-85f0-4c0b-815b-0443ac9edd15

In the meantime, we could either use a non-native Spring, or two different Spring (native for the circle, props for the label).

Edit: react-spring 5.6.8 allows to interpolate children, so we just have to upgrade :-)

This comment has been minimized.

@deamme

deamme Aug 22, 2018

Contributor

I've looked at the examples and tried to replicate it. It's really weird but I can't seem to get it to work - I've updated the react-spring etc.. My changes to CircleGraph: https://gist.github.com/deamme/67929fb11c9a7582a2d258cfe23a4f96

This comment has been minimized.

@bpierre

bpierre Aug 23, 2018

Member

I checked, it’s because react-spring is using innerText to update children, which only works on HTML elements, not SVG. We could either wait for an upcoming react-spring release, or use an HTML element for the label.

This comment has been minimized.

@deamme

deamme Aug 23, 2018

Contributor

Did a hacky thing with transform translate. https://gist.github.com/deamme/67929fb11c9a7582a2d258cfe23a4f96

This comment has been minimized.

@bpierre

bpierre Aug 23, 2018

Member

I think we could:

  • Set the dimensions of the parent div using size.
  • Use an HTML element (p or div) for the label.
  • Use absolute positioning for the SVG and the centered label.

That way it would properly adapt to size 👍

This comment has been minimized.

@deamme

deamme Aug 23, 2018

Contributor

Couldn't seem to get the p or div to work for label. I'll try the other options

This comment has been minimized.

@bpierre

bpierre Aug 23, 2018

Member

Demo with an HTML element => https://codesandbox.io/s/q3vp31z04q

This comment has been minimized.

@deamme

deamme Aug 23, 2018

Contributor

Ah I see, thank you!

@@ -0,0 +1,35 @@
# Table

This comment has been minimized.

@bpierre

bpierre Aug 22, 2018

Member

Table => Slider

@@ -17,6 +17,9 @@ export const springs = {
slow: { tension: 150, friction: 18 },
normal: { tension: 190, friction: 22 },
fast: { tension: 220, friction: 24 },

// Specific springs
slider: { tension: 400, friction: 28 },

This comment has been minimized.

@bpierre

bpierre Aug 22, 2018

Member

Maybe we could name it in a similar way to the new springs (“lazy” and “smooth”)… maybe something like “snappy”, “swift” or “stiff”? What do you think?

This comment has been minimized.

@deamme

deamme Aug 22, 2018

Contributor

I like swift. Let's do that

@@ -0,0 +1,31 @@
import React, { Component } from 'react'

This comment has been minimized.

@bpierre

bpierre Aug 22, 2018

Member

Thanks for also adding the page!

@deamme

This comment has been minimized.

Copy link
Contributor

deamme commented Aug 22, 2018

@bpierre Can't seem to get the DropDown to work with onRest.. Can you give me an example on how you would've done it?

@bpierre

This comment has been minimized.

Copy link
Member

bpierre commented Aug 23, 2018

@deamme I forgot that we could simply use Transition to do that: https://gist.github.com/bpierre/73bbe56bc05892a35c2e2d17086150e0

I also added a <BlockingLayer /> element to prevent any click during the closing animation.

@deamme

This comment has been minimized.

Copy link
Contributor

deamme commented Aug 23, 2018

@bpierre I know why things has failed for me regarding the DropDown component. To fix #169 I just deleted the position absolute styling on DropDownItems. This will do something weird to the animation though.. I've still applied your changes so the Transition will be used instead of Spring

@deamme

This comment has been minimized.

Copy link
Contributor

deamme commented Aug 23, 2018

Slider is acting weird in gallery and I haven't found a new fix for #169 yet

@bpierre

This comment has been minimized.

Copy link
Member

bpierre commented Aug 25, 2018

@deamme For the Slider, it’s because I was using document, which refers to the global document, but in that case we want the document inside of the iframe.

Using element.ownerDocument solves the problem: https://git.io/fAmvE

About #169, we can fix it separately from this PR (the solution would be to set a higher height on the iframe).

@deamme

This comment has been minimized.

Copy link
Contributor

deamme commented Aug 26, 2018

@bpierre Thanks again! I'll make a separate PR for #169 then. This one should be alright now

EDIT:
Found out this can also be fixed with removing overflow: hidden on the body - I don't know how this will affect other components though.

The ownerDocument solution introduces another problem when switching from the Slider component to another component in gallery. I get the following error: Uncaught Error: A cross-origin error was thrown. React doesn't have access to the actual error object in development. so this might only affect things in dev mode.

bpierre added some commits Aug 26, 2018

@bpierre

This comment has been minimized.

Copy link
Member

bpierre commented Aug 26, 2018

@deamme Oh, it’s because ref can be called with null. Fixed: 1904e41

I also removed react-motion from the dependencies.

Merging now 🎊

@bpierre bpierre merged commit 9fe0148 into aragon:master Aug 26, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Aug 29, 2018

This is awesome @deamme!

@sohkai sohkai changed the title Migration to react-spring and minor fixes (WIP) Migration to react-spring and minor fixes Nov 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment