Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Tooltip misplacement fixes #14767

Closed
wants to merge 4 commits into from
Closed

Tooltip misplacement fixes #14767

wants to merge 4 commits into from

Conversation

Saranya-Raaj
Copy link

Fixes #14756.
Successor to #14763.

Fixed jsbin

@hnrch02 hnrch02 added the js label Oct 10, 2014
@hnrch02 hnrch02 added this to the v3.2.1 milestone Oct 10, 2014
@hnrch02
Copy link
Collaborator

hnrch02 commented Oct 10, 2014

Don't mind the git issues for now.
The tests you added still don't fail on current master, meaning that there is (at least according to the unit tests) no improvement from our current code to your proposed changes.

cvrebert
cvrebert reviewed Oct 10, 2014
View reviewed changes
js/tooltip.js
placement == 'top' && pos.top - containerDim.scroll - actualHeight < containerDim.top ? 'bottom' :
placement == 'right' && pos.right + actualWidth > containerDim.width ? 'left' :
placement == 'left' && pos.left - actualWidth < containerDim.left ? 'right' :
placement = placement == 'bottom' && pos.bottom + actualHeight > containerDim.bottom ? 'top' :
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please adjust the spacing so that the operators remain vertically aligned, like they were in the old version.

@hnrch02
Copy link
Collaborator

hnrch02 commented Oct 10, 2014

@cvrebert If you pull down the changes with git fetch origin pull/14767/head:pr-14767 && git checkout pr-14767 and then diff with master do you see the changes in the unit tests? I'm baffled as to why GitHub shows the file as changed here but git reports no change locally. Edit: Never mind, if I git show 746eacd48dbbfa5e98a1c418039da846d8d21508 the change is displayed...

@Saranya-Raaj
Copy link
Author

@hnrch02 r
In the current master build there are no unit test cases handling the toolitp position in scrollable view port

@hnrch02
Copy link
Collaborator

hnrch02 commented Oct 10, 2014

Yes, but you aren't just adding tests you also fixed a bug and that bug would need to be caught by a new test so that there won't be any regression about it.

@Saranya-Raaj Saranya-Raaj force-pushed the tooltip_misplacement_fixes branch from b4897b5 to d2cd613 Compare October 10, 2014 14:18
@cvrebert cvrebert mentioned this pull request Oct 22, 2014
Closed
@hnrch02 hnrch02 closed this in e2cfbd5 Oct 22, 2014
@hnrch02
Copy link
Collaborator

hnrch02 commented Oct 22, 2014

Thanks for contributing again, @Matrixz!

@hnrch02 hnrch02 mentioned this pull request Oct 22, 2014
Closed
hnrch02 added a commit that referenced this pull request Oct 22, 2014
@hnrch02
Fix IE8 unit test breakage introduced by #14767
3c98dd3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
js
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.

Tooltip misplacement with "top auto" in scrollable viewport
3 participants
@Saranya-Raaj @hnrch02 @cvrebert