Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

fix(dropdown): align position with or without vertical scrollbar #6008

Closed
wants to merge 2 commits into from
Closed

fix(dropdown): align position with or without vertical scrollbar #6008

wants to merge 2 commits into from

Conversation

dolevd
Copy link
Contributor

@dolevd dolevd commented Jun 14, 2016

Fixes #5942.

Plunkr showing the fix:
http://plnkr.co/edit/jlBJjMgpDnR133ByUATF?p=preview

Resubmitted after fixing eslint issues.

Some notes regarding the added test:

  1. I've added a test that verifies that dropdown-menu-right works correctly when append-to-body is used. This test requires a real browser (since it must calculate the trigger and menu positions), so you may not wish to include it... On the other hand, it might be better than nothing.
  2. The test doesn't cover the case where a vertical scroll bar is present. As far as I can tell, this is very problematic to test since it requires a real environment where we can correctly calculate the width of the scrollbar. This isn't the case when running in Karma.

dolevd
dolevd reviewed Jun 14, 2016
View reviewed changes
src/dropdown/dropdown.js
@@ -195,7 +195,8 @@ angular.module('ui.bootstrap.dropdown', ['ui.bootstrap.position'])
var pos = $position.positionElements($element, self.dropdownMenu, 'bottom-left', true),
css,
rightalign,
scrollbarWidth;
scrollbarPadding,
scrollbarWidth = 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the eslint issue had to do with using tabs instead of spaces (I used Notepad++ which isn't the best tool for the job).
Eslint passes now, but I can change this if you wish.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah, not a big deal

@wesleycho
Copy link
Contributor

On the contrary, having a test is only a good thing!

This PR looks good to me, but can you explain what am I supposed to see in the Plunker from #5942 and this one?

@dolevd
Copy link
Contributor Author

dolevd commented Jun 14, 2016

In the Plunker from this PR you should hopefully see nothing out of the ordinary :).

The Plunker from #5942 shows that the dropdown with append-to-body and dropdown-menu-right is offset to the right by the width of the scrollbar when the scrollbar isn't visible.
In Edge and Safari, the scrollbar width is 0, so this doesn't have any effect.
However, in Chrome the normal scrollbar width is 17px, so you should see that the first dropdown is offset by 17px to the right (instead of being aligned to the right edge of the trigger element).

This happened because my original fix (that landed in 1.3.3) was very naive and simply assumed that the scrollbar is always present which obviously isn't the case.

@wesleycho
Copy link
Contributor

I actually am not able to reproduce the issue in Chrome (OS X) - what is your OS that you are seeing the issue on, Windows?

@dolevd
Copy link
Contributor Author

dolevd commented Jun 14, 2016 •
edited

Yes, windows 10.
This is a screenshot from my computer: http://imgur.com/tWBhIh1

@dolevd
Copy link
Contributor Author

dolevd commented Jun 14, 2016

I think OSX has scrollbars that are overlaid on top, so they probably don't change the window size and affect the right alignment.

@wesleycho
Copy link
Contributor

Alright, I'll verify when I get home a little later and likely merge then after verification

@wesleycho wesleycho added this to the 2.0.0 milestone Jun 14, 2016
wesleycho
wesleycho reviewed Jun 14, 2016
View reviewed changes
src/dropdown/test/dropdown.spec.js
@@ -702,4 +702,33 @@ describe('uib-dropdown', function() {
});
});
});

describe('using dropdown-append-to-body with dropdown-menu-right class', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

In a comment before this issue, can you mention // issue #5942?

@wesleycho
Copy link
Contributor

Sorry for taking so long - been traveling the past two weekends.

This LGTM.

@wesleycho wesleycho closed this in 2d831bc Jun 27, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
component: dropdown
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.

Right alignment is off for dropdown-append-to-body .dropdown-menu-right when there's no vertical scrollbar
2 participants
@dolevd @wesleycho