Expand animation

[nathanmarks]

I understand that it requires a JS based solution -- is expanding height animation something that you want as part of this library?

From my understanding, the jerky feel of the current expanding behaviour for cards, sidenav, etc... goes against the material design philosophy of natural, authentic motion & interaction.

Is this something you have put any thought to, if so, do you have a preferred implementation?

The most common method involves calculating the expected expand height with JS, and using that to animate a height or margin property to achieve the affect.

[mbrookes]

is expanding height animation something that you want as part of this library?

It's already part of Popover. What component did you want to animate?

the jerky feel of the current expanding behaviour

In what browser / version, on what platform, with what version of Material_UI and React? (You did read the contributing guide before posting, right? 😁

[mbrookes]

Damned "Close and comment" button! 😄

[nathanmarks]

@mbrookes My apologies for not following the guidelines. I'll ensure that they are followed in the future!

To be specific, I'm referring to height expansion in the side nav and card expanders (ignore crappy gif fps): http://g.recordit.co/RSo7NEISi6.gif

Seen on: https://www.google.com/design/spec/animation/authentic-motion.html

From that page:

In the physical world, forces must be applied to an object in order for it to move. The strength and duration of these forces dictate how quickly an object accelerates, decelerates, or changes direction. Even the most jarring stops and starts are not instantaneous, because it takes time for an object to speed up or slow down. Consequently, when animations have abrupt starts, stops, or changes in direction, they appear unnatural.

[nathanmarks]

To be clear, I understand that these are not currently animated -- hence the issue.

[mbrookes]

Ah, when you said "the jerky feel of the current expanding behaviour", I thought you meant some existing animation.

Yes, animation of the ListItem NestedItems would be nice. I'm sure this came up recently in discussion (or maybe I just thought it :).

I'm not that familiar with the internals of how various components animate, but you could take a look at styles/transitions.js and the LeftNav animation for some inspiration.

[nathanmarks]

@mbrookes I've implemented vertical height expansions on a number of occasions -- and they all involve some sort of precalculation to assess the height requirement since you cannot animate height: auto.

Looking at Google's own implementations, this is also what they do.

As this involves interfacing directly with the DOM, I wanted to see if there were any opinions from the maintainers on implementation details.

[mbrookes]

I've implemented vertical height expansions on a number of occasions [...]
you cannot animate height: auto

No, but you can animate max-height: http://codepen.io/mbrookes/pen/GowvKN

[nathanmarks]

@mbrookes The problem with animating max-height is that it animates exactly that - max-height.

This means that if your max-height is 1000px and your container is 100px, not only is there a delay before the animation starts when contracting, but it only starts when max-height is at 100px and you miss 1/10th of the animation time

[nathanmarks]

That also means that your transition easing, designed for a natural "material" feel, will not be in sync with the visible animation.

[newoga]

@nathanmarks I agree that supporting animations for <List /> and <ListItem /> components would be nice. New features are not on the priority list for us at this current moment because we're doing a lot of refactoring/cleanup. But that doesn't mean we won't consider proposals and PRs. 😄

Do you have some time to work on this?

[nathanmarks]

@newoga Next week I will, what are your thoughts regarding the implementation?

@mbrookes I urge you to check out examples in the wild, Google use the calculation + height animation technique in their own products/services

Before click:
https://dl.dropboxusercontent.com/s/xwfcslg8s0s32ey/2016-02-08%20at%208.44%20PM%20%281%29.png

After click:
https://dl.dropboxusercontent.com/s/dghzqqtvwztj4j4/2016-02-08%20at%208.45%20PM.png

[mbrookes]

not only is there a delay before the animation starts when contracting, but it only starts when max-height is at 100px and you miss 1/10th of the animation time

That also means that your transition easing, designed for a natural "material" feel, will not be in sync with the visible animation.

Mmm, both good points. 🙇

If you have a workable solution I'm sure I'm not the only one who would appreciate it. That snappy ListItem NestedItems has been bothering me too. 😄

[nathanmarks]

@mbrookes Totally agree, it ruins the lovely feels all the other animations give me. The solution for ListItem > NestedItems should be relatively simple to implement and I can do soon. The Card expander is a bit more complex so maybe the two features should be treated separately.

[newoga]

@newoga Next week I will, what are your thoughts regarding the implementation?

Awesome! I'm looking forward to it and really appreciate you taking it on.

Like you suggested earlier, I think we're going to have to use javascript to apply inline styles to get the right animation behavior here. Beyond that I'm open to you taking the lead and giving it a shot 😄. That being said, you're not alone. Feel free to keep the dialogue open and run things by us along the way if you hit a snag. I think starting with the simpler of the two (List/ListItem) is a good call and maybe we'll learn something along the way that we can apply to other components.

A few general thoughts as it relates to PRs: as I said right now we're trying to streamline and simplify the codebase. I think approaches that would allow the animation implementation to be swapped, customized, externalized, etc are preferred. Maybe also investigate approaches this problem could be solved partially (or completely) user space using existing animation tooling for React.

That being said, don't stress over abstracting from the start, if you get an implementation that is more hardcoded and coupled, we can look and investigate approaches to externalizing it and making it more re-usable.

Also since everyone right now on the project is part-time, demos and/or examples of how it works helps tremendously when it comes time to review. Below are some recent examples that have been helpful:
Stepper: #3132
Slider: #3237

Recently I event created a new respository to experiment approaches and used material-ui as I dependency. (You don't have to do this, but if you would rather, feel free to do that then we can create a PR later):
https://github.com/newoga/material-ui-scrolling-techniques

Hopefully that helps! 😄

[nathanmarks]

@newoga Sounds good.

Writing accompanying tests to confirm expected behaviour is also a good idea.

[newoga]

Writing accompanying tests to confirm expected behaviour is also a good idea.

Music to my ears, thanks much @nathanmarks 👍

[nathanmarks]

@newoga

What's your preference?

a) new repo as you mentioned, create functionality almost as a 'plugin' that is completely modularized
b) branch on a fork

I would tackle the problem in the same way, regardless of which option -- I was just wondering if you were proposing option A for any other reason other than forcing to think in a modular fashion.

PS, as a side note: Would I be right in assuming that the test setup hasn't received a lot of love? I was a little surprised to see only phantomjs based tests. The reason I say that is because if you had stronger test coverage (a number of components currently not tested) it'd become more apparent that running phantomjs tests, especially with that browserify build is pretty slow. Generally, you can cover a lot of functionality with shallow rendering and leave the phantomjs testing for where it really matters to confirm real DOM behaviour (later this can also be expanded to cross browser tests via selenium).

[newoga]

I would tackle the problem in the same way, regardless of which option -- I was just wondering if you were proposing option A for any other reason other than forcing to think in a modular fashion.

Yes that was pretty much the only reason. I really don't have a preference, whichever makes it easier for you! If you can manage with a fork and a branch, then that's probably best in terms of ease and setup.

Would I be right in assuming that the test setup hasn't received a lot of love? I was a little surprised to see only phantomjs based tests.

I can't comment on how it's been received by others, but I definitely agree it needs a lot of work! This was something I wanted to investigate after we reorganized the modules (#2706) (#3212), but if it's something you want to take a stab at I'd be eternally grateful. 😄 👍

[nathanmarks]

Okay thanks!

What did you have in mind for your own plans RE the test suite?

I think that the following would be good goals:

• Unit testing via shallow rendering as it is very fast therefore more appropriate for TDD (better way to confirm expectations while developing than inspecting the documentation site).
• Reserve phantomjs tests for integration testing and other DOM dependent behaviour. This also leads the way for a dedicated selenium test suite later down the line for automated cross-browser testing.
• Webpack to replace browserify (you're using it for the docs, so makes sense as upgrading is easier with 1 module loader configuration style to deal with).
• Super easy to run unit tests using a specific component name

[newoga]

Unit testing via shallow rendering as it is very fast therefore more appropriate for TDD (better way to confirm expectations while developing than inspecting the documentation site).

👍 Test speed/performance was one of my current issues with our current setup.

Reserve phantomjs tests for integration testing and other DOM dependent behaviour. This also leads the way for a dedicated selenium test suite later down the line for automated cross-browser testing.

👍

Webpack to replace browserify (you're using it for the docs, so makes sense as upgrading is easier with 1 module loader configuration style to deal with).

👍 👍

Super easy to run unit tests using a specific component name

Easier the better. There was a chance we were going to move component tests in the same directory as the component after the migration, I'm not sure if that is still on or off the table. Though I wouldn't let that be a factor yet because it should be easy enough to change.

@nathanmarks Improving the testing story for this project would be a huge win. No pressure but I'm looking forward to what you got.

[nathanmarks]

@newoga I'll see what I can come up with.

This looks pretty good by the way in terms of providing some quick and easy unit test rendering solutions: https://github.com/airbnb/enzyme

Comes with a shallow renderer and "full" (still virtual, jsdom) renderer. Jsdom is a nice middle-ground between shallow rendering and full browser rendering via phantomjs, selenium, etc.

There's a good overview of the functionality in the readme and some more details on the shallow renderer and full renderer.

I'm familiar with the core tools they use under the hood for the shallow and full renderer -- both are top notch.

Obviously the different parts needed for the test suite can be stitched together manually, but this work from airbnb can save quite a bit of time (for the simple unit tests), what do you think?

FYI, I've started using this in a work project that was previously using another shallow renderer + jsdom.

[newoga]

@nathanmarks That looks interesting! I haven't used it but it looks nice. I agree this could help simplify and save time for a majority of the scenarios we're testing. I'll try to look more in depth later and try it out, but go ahead and give it a stab if you think it will help. 👍 (in matter of fact, I see that you already have in your repo 😄)

[oliviertassinari]

This means that if your max-height is 1000px and your container is 100px, not only is there a delay before the animation starts when contracting, but it only starts when max-height is at 100px and you miss 1/10th of the animation time

That's exactly what is going on with our Show source button in the documentation. Not great 👎.

I'll see what I can come up with.

We are really interested in what you came up with 👍. For anyone how is interested in this topic, this post http://reactkungfu.com/2015/07/approaches-to-testing-react-components-an-overview/ is really great. It give a give an overview of the different use cases / solutions / pros / cons.

[nathanmarks]

@newoga Haha, I've started setting up in the /test folder.

Right now the structure is:

test/
test/browser
test/unit

For now, the /browser subfolder will contain the current phantomjs tests. These will be able to be run using npm run test (all tests) or npm run test:browser.

The /unit folder will contain unit tests that use the shallow and full/virtual renderers found in enzyme. This unit test suite won't require any bundling and will run using the babel require hook. These tests will be run using npm run test or npm run test:unit. There will also be a watch option and the ability to run tests by component name.

On the note of component names, I noticed that the src/ folder is a bit of the wild wild west right now. What is the plan moving forwards? Are component files all going to be moved to PascalCase? I checked the issues you linked but discussion seems to have tapered off without a conclusion yet.

If the plans for a more permanent solution are a way off from happening, It would be nice if there was an src/components.js that had a named export for every component with a flat structure -- even if just for testing purposes.

[nathanmarks]

@newoga Also, what's the preference around here? Do you want me to stick with mocha for the unit test framework too? I was going to run with that since you are already using it for the karma+phantomjs tests too. Giving you the opportunity to speak up now if you want to use a different test fw! 🎱

[oliviertassinari]

Right now the structure is:

I think that it would be good in the future to move the tests close to the source code (as the docs and the examples and the native implementation). But that is depending on another issue.

Are component files all going to be moved to PascalCase?

We hope so but that's work in progress. For instance: #3212.

[nathanmarks]

@oliviertassinari PS, the shallow renderer used in airbnb/enzyme is not the react shallow renderer, and allows for additional functionality (simulating events, traversing, etc)

[nathanmarks]

@oliviertassinari If you can give me some pointers as to where the structure is going, I can set up the unit tests in __tests__ directories.

Perhaps the browser tests should stay separate though as they will also be used for integration testing.

[newoga]

I think that it would be good in the future to move the tests close to the source code (as the docs and the examples and the native implementation). But that is depending on another issue.

I'd also really like to move the tests closer to the components. Though since we have so few, I'd say let's keep the structure as is until that PascalCase/directory migration is done. We can sort out moving test code during that process.

@nathanmarks I think your structure is good for now. I would just follow keep the test directory in sync with what's in src. The plan is to do all the directory stuff before releasing 0.15.0 (and hopefully really soon). When it's moved, I agree that browser tests still might have to be a more "global" location.

[newoga]

@newoga Also, what's the preference around here? Do you want me to stick with mocha for the unit test framework too?

@oliviertassinari @alitaheri Any thoughts on switching this? I'm fine with mocha and don't have any strong feelings. I started with Jest when I started with React but I have used and seen Mocha pretty much everywhere else.

[nathanmarks]

@newoga Yeah, I would stick with Mocha in that case. Heard bad things about Jest. I use tape or mocha for the test framework.

[newoga]

Sounds good, let's stick with it then!

[oliviertassinari]

What do you guys think about using https://github.com/souporserious/react-measure for animating the height?

[nathanmarks]

@oliviertassinari I think that it may be a case of bringing a gun to a knife fight. I'm not sure we need mutation observers for this.

[newoga]

Just dropping a quick note here:

@pradel offered to help write tests for components that are missing them after @nathanmarks is finished with the test reorganization. Thanks guys, this is really going to help push this project forward!

[nathanmarks]

@newoga awesome!

I'll be able to finish getting things setup over the weekend.

[newoga]

Sounds great, thanks @nathanmarks.

[nathanmarks]

@newoga

Is your preference to split the coverage reports between the browser and unit tests or try merge them? (the unit tests won't be using karma or a webpack bundle, so they'd need to be merged if you want them combined).

[nathanmarks]

BTW, the current coverage is very misleading as it only covers the files the tests cover. And the only components with good coverage are SVG icons 👎 (special mention for paper, and a couple of style helpers)

https://dl.dropboxusercontent.com/s/iu1wsnp7qvku9tf/2016-02-12%20at%208.14%20PM.png

Think @pradel will need some help filling in the gaps :)

[newoga]

@nathanmarks I've got no problem keeping the reports separate for now! Yeah this area needs a lot of work :(. In addition not having good tests hasn't done much for developing a good test culture for the project. As much as I'm also looking forward the the list item animation, I'm really glad this area is getting tackled.

Think @pradel will need some help filling in the gaps :)

I agree, it'll have to be a group effort to get tests for all these components. I think it would be a good goal to get a basic level of coverage for all the components for our 0.15.0 betas. Either way, I think we're heading in the right direction 👍

[nathanmarks]

@newoga I don't see any value in including src/svg-icons/* in coverage reports. It takes the coverage report from around 9,000 to 23,000 statements and almost doubles the line count. Thoughts?

[newoga]

@nathanmarks I agree. Funny you should mention, I was just discussing the topic of icons. Right now I'm advocating that we convert the project to a monorepo and put the icons in their own material-ui-icons package when we do the directory reorganization. Everything gets slower/more bulky with icons: build, running docs site, tests, etc. and it's mostly generated and static code. So I think it makes sense to remove it from coverage.

[nathanmarks]

👍 Great!

PS, should we take discussion regarding the testing elsewhere? this "expand animation" issue has turned into a monster. (or rename this issue, or something)

[mbrookes]

Good Idea, if you open a new umbrella issue and outline the plan, we can add it to the roadmap, and close this: #1973.

[nathanmarks]

@mbrookes #3330

[newoga]

Thanks!

[newoga]

@mbrookes Do you think we should just close this issue too and start a new one from scratch that deals with specifically animating the nested list item closing/expansion?

[mbrookes]

Makes sense. @nathanmarks since you proposed this, I'll let you post a new issue, then I'll close this one.

[nathanmarks]

@mbrookes #3392

[oliviertassinari]

@nathanmarks Thanks.

[MarcMagnin]

Hey guys, sorry to revive the monster by what about that animation?

[Panoplos]

Any progress on this one?