Allow the react component animation duration to be configurable. Related to #69. - #93
Allow the react component animation duration to be configurable. Related to #69.#93raphaabrasil wants to merge 1 commit into
Conversation
1egoman
left a comment
1egoman
left a comment
There was a problem hiding this comment.
Other than the things that I mentioned in the review, could you potentially add a test or two to test the the animation duration?
Lastly, the tests are breaking in CI. They can be run locally with npm test, and here's a link to the run on travis: https://travis-ci.org/1egoman/funnies/jobs/284427154
Thanks for the contribution!
| leave: this.state.transitionTimeout.leave, | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Can you make this one line and add a space after if? } else if (type === 'leave') {
| padding-top: 0.5em; | ||
| padding-bottom: 0.5em; | ||
| min-height: 2em; } | ||
| .funnies-wrapper--input-container { |
There was a problem hiding this comment.
What does the double dash signify? Personally I prefer single dashes for the whole thing (ie, funnies-wrapper-input-container) but I'd entertain alternatives if there's a compelling reason to use them.
There was a problem hiding this comment.
It's just a pattern (or a methodology) that i like to use because i think that it combines very well with SASS called BEM.
By the way, i used the wrong pattern. According to BEM, in this case, i should have used .funnies-wrapper__input-container instead of .funnies-wrapper--input-container
There was a problem hiding this comment.
Ok, cool. I don't really mind it, was just curious. Thanks for the link!
| this.setState({ interval: parseInt(value) }); | ||
| }, | ||
| updateTransitionTimeout(type, value) { | ||
| if(type === 'enter') { |
There was a problem hiding this comment.
Can you add a space after if? if (type === 'enter') {
|
Ok, I'll fix those things and write some tests! |
2 participants
No description provided.