-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Fix transition for Replace ModalBar with 2 lines #7524
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1087,6 +1087,17 @@ a, img { | |
} | ||
} | ||
|
||
&.offscreen2 { | ||
-webkit-transform: translate(0, -88px); | ||
transform: translate(0, -88px); | ||
transition: -webkit-transform 266ms cubic-bezier(0, 0.56, 0, 1); | ||
transition: transform 266ms cubic-bezier(0, 0.56, 0, 1); | ||
|
||
body:not(.has-appshell-menus) & { | ||
top: 81px; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had a hard time trying to figure why this was changed, until I realized that this changes because the menubar will use 2 lines, one for the menus and 1 for the title. It might be better if we set this value in JavaScript. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @TomMalbran No, this is when the ModalBar takes 2 lines for FindReplace: first line is for Find text, second line is for Replace text (and other assorted widgets). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure? Notice that this is the top property and applied only when there are html menus. The idea behind this was to change the position property of the modalbar from static to absolute so that the editor could be placed under the modalbar. If we didn't do this the modalbar would leave a blank space where it used to be instead of showing the editor. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that I remember, I helped larz with the original implementation and this line was to fix the position with the html menu bar. More on that in the original PR: #4684 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is used to position the modalbar before starting the animation. Without html menus is placed at (0,0) using an absolution position relative the editor holder. With html menus is has to be placed a below the menus. This bit of code should be placed inside the Anyway that top still has an issue. The html menu can become 2 lines too and the height is higher. This is why I think that we should just change the style in JavaScript and remove that from the code. This can be done in If we then change the |
||
} | ||
} | ||
|
||
input { | ||
font-family: @sansFontFamily; | ||
outline: none; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -183,7 +183,8 @@ define(function (require, exports, module) { | |
*/ | ||
ModalBar.prototype.close = function (restoreScrollPos, animate) { | ||
var result = new $.Deferred(), | ||
self = this; | ||
self = this, | ||
animateClass; | ||
|
||
if (restoreScrollPos === undefined) { | ||
restoreScrollPos = true; | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
AnimationUtils.animateUsingClass(this._$root.get(0), animateClass) | ||
.done(doRemove); | ||
} else { | ||
doRemove(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
-100%
seems to work fine.