Lift component improvements (values + animation) from MM #1330 #450
Conversation
edhenderson
commented
Jul 14, 2016
•
edhenderson
commented
- added a max and min lift (2 == 200%)
- curtailed our lift values
- added animation
- smarter variable names
edhenderson
changed the title
edhenderson
force-pushed
the
eh-1330
branch
2 times, most recently
from
d835cf6
to
ae77477
Compare
|
@edhenderson LGTM |
|
❄️ HOLD for now. |
edhenderson
force-pushed
the
eh-1330
branch
2 times, most recently
from
9d30a35
to
aba5227
Compare
|
@apapirovski I don't want to merge this one just yet. The animation is looking a little janky. Its most likely the way I implemented it OR your example only had 1 hot thumb to mouse over. Can you take a look at this branch and give me your thoughts please? |
| <ReactCSSTransitionGroup transitionName="xxFadeInOutSequential" transitionEnterTimeout={400} transitionLeaveTimeout={400}> | ||
| <div className="xxLift-container" key={`lift-${classicLiftPercent}-${neonLiftPercent}-${originalLiftPercent}`}> | ||
| <strong className="xxLift-title">{T.get('copy.lift.units', { | ||
| '@lift': classicLiftPercent |
There was a problem hiding this comment.
This should always show the real Lift value, not the limited one.
|
I see what you mean about the jank. Working on a fix. Will commit to this branch if you're ok with it? |
|
Were you able to see the animation in action? |
|
Crossed beams. Thanks. |
|
Whatever your fix. Needs to work in the xx sandbox at least and bonus here too. Cheers. |
|
@apapirovski I pushed a tweak (in case your working on this). |
| neonLift, | ||
| neonLiftPercent | ||
| // rawLift = isNaN(self.props.displayThumbLift) ? 0 : self.props.displayThumbLift, | ||
| rawLift = 30, |
There was a problem hiding this comment.
Testing value? Seems like you're not using the prop displayThumbLift.
There was a problem hiding this comment.
Ah yes, I forgot a commit to put it back.
|
@edhenderson I'm working on a fix. It's a bit more complicated because See facebook/react#5028 for more info. |
|
@apapirovski Might be worth pulling into a separate PR then? |
|
@edhenderson Maybe. If I have to do anything more complicated, I'll create a separate PR. If it's just a matter of some CSS or using something like react-motion for this particular transition then I think it would be fine to keep in this one. (To avoid needing you to rebase, integrate changes, etc.) Your call though. |
|
This won't be merged till the PM at the earliest so see what you got by then. |
|
Not perfect but this should work 99.9% of the time. The alternatives — such as creating our own fork of I might work on my own library for handling CSS transitions in React, which can be directly swapped with |
|
OK, so GTG now? |
edhenderson
changed the title
|
Yeah, I think it is. |
|
OK, when I merge it, if you got time, can you pull development and check you are happy with it. (I need to rebase). |
- added a max and min lift (2 == 200%) - curtailed our lift values - added animation - smarter variable names - added animation constant - better variable names - don’t throttle the `displayLiftPercent`, i.e. use `rawLift` before its constrained
