-
Notifications
You must be signed in to change notification settings - Fork 7.5k
Fix sizing and scrolling issues from latest upstream-master #2658
Conversation
|
FYI, #2657 still repros in this branch--you might have to resize the window narrower and then wider a few times to see it. |
|
Reviewing |
src/editor/Editor.js
Outdated
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.
Since we're now calling this on every update of the inline editor, we should be careful that we're not doing any work we don't need to. I would hope this is a no-op if the height hasn't changed, but to be safe, could we guard this inside the if (changed) block?
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.
fixed
|
Review done. Looks good and seems to work--just the one comment. |
|
@njx ready to review |
src/editor/Editor.js
Outdated
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.
Actually--this should still (also) be in the if (changed), right?
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.
Fixed
|
Re-reviewed, still needs one tweak. |
src/editor/Editor.js
Outdated
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.
Still not quite right, sorry :) -- this will make it so we bail (in the else below) if changed is false even if inlineWidget.info is defined, so we would skip executing the ensureVisible code (which we should only skip if inlineWidget.info is undefined).
|
Just one more thing... |
|
@njx Whoops. Thanks for pointing that out. |
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.
Missing comma
|
Fixed the last little bit. Merging. |
Fix sizing and scrolling issues from latest upstream-master
@njx Fixes #2655, #2523, #2521.
redrawevent on the widget (from the host editor) and refresh inline instances of CodeMirrorsizeInlineWidgetToContents()calls into anupdateevent listener. Whenever an inline editor update's it's display, we force a new height on the inline widget containerI'm also seeing #2657 fixed in my branch. But I'm not sure how my changes would have affected horizontal scrolling.