Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Possible bugs in MultiRangeInlineEditor #2220

Closed
njx opened this issue Nov 27, 2012 · 5 comments
Closed

Possible bugs in MultiRangeInlineEditor #2220

njx opened this issue Nov 27, 2012 · 5 comments
Assignees
Milestone

Comments

@njx
Copy link
Contributor

njx commented Nov 27, 2012

Discovered by inspection while fixing #2218. Too risky to fix right now (end of sprint 17) but we should look at them for sprint 18.

  1. _updateRelatedContainer() is both attached as a handler for the host editor "change" event and called from _handleChange(), which seems redundant.
  2. In _updateRelatedContainer(), the width calculation uses lineSpace.scrollWidth. It seems like this should be either lineSpace.width or this.hostEditor.getScrollerElement().scrollWidth.
@peterflynn
Copy link
Member

We could also handle the 2nd issue via #2221, where we'd need to reconcile MultiRangeInlineEditor with the simpler code used in InlineColorEditor.

@njx
Copy link
Contributor Author

njx commented Nov 27, 2012

As @peterflynn pointed out elsewhere, it's also not clear why the width calculation needs to be so complicated--it seems like it could just use the scroller's scrollWidth, as we do in InlineColorEditor.

@pthiess
Copy link
Contributor

pthiess commented Nov 28, 2012

reviewed and assigned to @njx , sounds like there was potential for refactoring.

@njx
Copy link
Contributor Author

njx commented Dec 5, 2012

Turns out (1) isn't a bug...one of them is on the host editor's change event, while the other is on the change event of the inline editor.

@njx
Copy link
Contributor Author

njx commented Dec 5, 2012

Turns out there's a reason for the complex width calculation in MultiRangeInlineEditor--the simplistic thing we're doing in InlineColorEditor doesn't work (see #2218). Additionally, if I change lineSpace.scrollWidth to something like $(lineSpace).outerWidth(), it doesn't seem to get the proper width for some reason. So I'm going to close this bug as NAB, do the refactoring in #2221 to port the MRIE calculation up to InlineTextEditor, and comment why the complex calculation seems to be necessary.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants