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

[FEATURE components] Add <Collapse> component #1114

Merged
merged 1 commit into from Aug 14, 2018

Conversation

@yuki24
Copy link
Member

yuki24 commented Aug 10, 2018

This adds a new <Collapse> component we are going to use in the new Buy Now flow:

collapse

@artsyit

This comment has been minimized.

Copy link
Contributor

artsyit commented Aug 10, 2018

Deploy preview for artsy-reaction ready!

Built with commit 1ce39e6

https://deploy-preview-1114--artsy-reaction.netlify.com

@damassi

This comment has been minimized.

Copy link
Member

damassi commented Aug 10, 2018

@yuki24 - Since #1082 we've started writing tests for all UI components; would you mind adding one before merge?

const computedHeight = getComputedStyle(this.element).height
this.element.style.height = prevHeight

this.setState({ computedHeight })

This comment has been minimized.

@damassi

damassi Aug 10, 2018

Member

One thought is that elsewhere in Reaction we've started using react-spring: https://github.com/drcmda/react-spring; that might be something to consider here and the css animation could be eliminated (though I do like the current simplicity -- but am wondering about long-term standardization on animation patterns across Artsy).

cc @l2succes

This comment has been minimized.

@yuki24

yuki24 Aug 10, 2018

Author Member

Nice, I did some research yesterday but totally missed react-spring! I'll look into that and see if we could take advantage of it.

This comment has been minimized.

@zephraph

zephraph Aug 10, 2018

Contributor

We're using react-spring and react-transition-group?

This comment has been minimized.

@zephraph

zephraph Aug 10, 2018

Contributor

Regardless, yeah, we really should look to standardize on animations... @eessex and @xtina-starr might have some insight too.

This comment has been minimized.

@damassi

damassi Aug 10, 2018

Member

Yup yup, react spring is being used in various places around Reaction

This comment has been minimized.

@l2succes

l2succes Aug 10, 2018

Contributor

@xtina-starr see my answer above but just to restate. react-transition-group is a DOM specific animation library. react-spring allows us to use the same animations and components in reaction and emission for instance. Also, having a simpler API to deal with would be a win for us in terms of developer accessibility.

The docs for react-spring has a good explanation for what problems it solves (https://github.com/drcmda/react-spring#why-do-we-need-yet-another-), You should check it out.

This comment has been minimized.

@orta

orta Aug 10, 2018

Member

Not to pour gasoline on the discussion, but do we have enough animations in our apps to warrant a dependency at all? Most of the cases I can think of (except maybe that "click here to inquire on an artwork" thing) are mostly just hiding/showing/fading things elegantly

This comment has been minimized.

@eessex

eessex Aug 10, 2018

Member

@damassi @l2succes as far as i could tell there was no option in react-spring to not mount a component that shouldn't be in view, although this is build into react-transition-group -- IMO hiding something with CSS only defeats the purpose of using a library, or using react with animations. I guess we could add this functionality to react-spring, but there didn't seem to be a point of adding functions to a library we had no other uses of.

This comment has been minimized.

@damassi

This comment has been minimized.

@briansw

briansw Aug 10, 2018

Libraries/dependencies aside – from a UX perspective, we want to be taking a more modern approach to interactions by making use of animation to clarify interactions, particularly complex interactions that change the page composition to ground the user in order to avoid confusion. Hope that's helpful.

@ds300
Copy link
Contributor

ds300 left a comment

Sorry to butt in 👋

Animating the height property is crazy bad for performance because it causes browser reflow at every frame. This will likely cause jank for a lot of people, even if not on our nice fast MacBooks.

Most people these days stick to animating opacity and transform, which are cheap to animate because they don't affect layout and often can be delegated to the GPU.

With those two properties you can achieve a really nice 'collapse'-like effect that won't get all janky on busy pages or less powerful machines.

Here's Airbnb's version in action:

airbnb fade

Snappy, right?

@alloy

This comment has been minimized.

Copy link
Member

alloy commented Aug 13, 2018

Great addition @ds300 👌 With regards to abstractions, is there a library that would have encouraged the performant option? Or is that hard to do and thus possibly meaning that we shouldn’t be using any off the shelf libs for our needs, as it may make it harder to review the actual implementation, especially because our needs are relatively little?

@ds300

This comment has been minimized.

Copy link
Contributor

ds300 commented Aug 13, 2018

I've heard great things about this: https://popmotion.io/pose/ it apparently errs on the side of performance and is quite flexible.

(Personally I've rolled my own on previous projects, sometimes with help from react-transition-group, though not convinced that's the best tradeoff in terms of time/bloat)

@yuki24 yuki24 force-pushed the yuki24:collapse-component branch from aeb12a1 to 2c35c49 Aug 14, 2018

@yuki24 yuki24 force-pushed the yuki24:collapse-component branch from 2c35c49 to 1ce39e6 Aug 14, 2018

@yuki24

This comment has been minimized.

Copy link
Member Author

yuki24 commented Aug 14, 2018

I ended up using react-spring since it has a universal component for React Native and it will make it easier to port this component when necessary.

I'm not that concerned about performance, but just FYI, react-spring is slower than the plain CSS transition compoment. Here is the performance breakdown captured using Chrome's performance feture with 8 collapse animations running simultaneously and the settings of CPU throttling 6x slowdown:

Plain CSS transition

The lowest FPS was 18 with a rough average of 35 FPS:

screen shot 2018-08-14 at 12 38 29 pm

react-spring

The lowest FPS was 9 with a rough average of 23 FPS:

screen shot 2018-08-14 at 12 46 03 pm

@yuki24 yuki24 force-pushed the yuki24:collapse-component branch 2 times, most recently from f7a0205 to 1ce39e6 Aug 14, 2018

@zephraph

This comment has been minimized.

Copy link
Contributor

zephraph commented Aug 14, 2018

I don't think we've really had a consensus on how to tackle this, but in consideration of @yuki24's time, I'm going to merge this so he can move on. I think we're just going to need an RFC for animation strategy.

@zephraph zephraph merged commit 0b318db into artsy:master Aug 14, 2018

6 checks passed

ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: relay Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: type-check Your tests passed on CircleCI!
Details
ci/circleci: update-cache Your tests passed on CircleCI!
Details
deploy/netlify Deploy preview ready!
Details
@ds300

This comment has been minimized.

Copy link
Contributor

ds300 commented Aug 14, 2018

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