Fix transition for Replace ModalBar with 2 lines #7524
Conversation
|
@larz0 Does this look OK? |
|
@redmunds I'll be able to check this out next week. |
|
@redmunds this is awesome. So much better now! |
|
UX review done. |
| @@ -210,7 +211,9 @@ define(function (require, exports, module) { | |||
| } | |||
|
|
|||
| if (animate) { | |||
| AnimationUtils.animateUsingClass(this._$root.get(0), "offscreen") | |||
| // Use "offscreen2" class if ModalBar is 2 lines tall, otherwise use "offscreen" | |||
| animateClass = (this.height() > 50) ? "offscreen2" : "offscreen"; | |||
There was a problem hiding this comment.
Where do you get the magic number 50? Wouldn't it be more reliable to check for the visibility of the second text field?
There was a problem hiding this comment.
The second text field is always visible -- we need to determine if it's on the first line or the second line.
50 is roughly a line and a half in height. The height can only be exactly 1 or 2 lines, so it doesn't need to be exact. This could be a problem when we add HiDPI support, though.
A better check might be to see if the top of the 2 text fields is the same, or if second field is below first field.
|
@TomMalbran I assigned you as reviewer on this one. |
|
@redmunds Sure. Do you want to try the easier fix using percentage and fixing the top position from JS, or should I open a new PR? |
|
@TomMalbran I've been busy with other stuff, so you're welcome to submit a PR. Thanks. |
|
#7743 is a better fix. Closing. |
This is for #7396.
For Replace, the modal bar can only ever be 1 or 2 lines, so I went with a simpler approach. I created a second
offscreen2class for closing a 2 line modal bar, and then conditionally apply the correct class.I don't see any problem with the transition that opens a 2 line modal bar, so This new class is only used for close.
cc @larz0