New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix inline text field position #927

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@nicolasbadia
Copy link
Member

nicolasbadia commented Feb 13, 2013

Since this commit: 4d19990 (which were welcome since besides fixing the z-index, it also fix a problem where if an inline text field was in a scroll view, the field wasn't moving when scrolling), the calculation of position of the editor was completely wrong. This can be see in the showcase, with the SC.LabelView and the SC.ListView examples. Based on my app and the showcase app, this commit fix the problem. I haven't added units test because I believe this can't be tested. Note that I've also dropped IE7 support for this since I'm not able to test it and because even Twitter Bootstrap will drop IE7 support in there next release.

Fix inline text field position
Since this commit:
4d19990
2de01b5f0576a907 (which were welcome since besides fixing the z-index,
it also fix a problem where if an inline text field was in a scroll
view, the field wasn't moving when scrolling), the calculation of
position of the editor was completely wrong. This can be see in the
showcase, with the SC.LabelView and the SC.ListView examples.
Based on my app and the showcase app, this commit fix the problem. I
haven't added units test because I believe this can't be tested.
Note that I've also dropped IE7 support for this since I'm not able to
test it and because even Twitter Bootstrap will drop IE7 support in
there next release.
@dcporter

This comment has been minimized.

Copy link
Member

dcporter commented Feb 19, 2013

Context-cum-disclaimer: I monkey-patched this in with a reopen-override of positionOverTargetView.

I haven't got a minimal test case (or the time ahead of a deadline to put one together), but when using this in the context of a panel pane (probably not a consideration), this takes a label view at { top: 0, left: 0, height: 24, right: 35 } and gives its editor a layout style of "right: 0px; left: -120px; top: -40px; height: 24px;", putting it above the view and substantially wider to the left. I don't believe I'm doing anything fancy or edge-case-y anywhere.

Fix review
Review of the positionOverTargetView method after dcporter report.
Removed blurOnMouseDown which is not needed anymore.
Add support for the multiline flag.
@nicolasbadia

This comment has been minimized.

Copy link
Member Author

nicolasbadia commented Feb 19, 2013

I reviewed the positionOverTargetView method. Hope everything's fine now.
I've also add support for the multiline flag when it's set to true on the labelView and remove blurOnMouseDown which is not needed anymore.

@publickeating

This comment has been minimized.

Copy link
Member

publickeating commented Mar 2, 2013

Thanks! Rebased this onto master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment