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.

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(select): prevent the panel from going outside the viewport horizontally #3864

Merged
merged 1 commit into from
Apr 21, 2017

Conversation

crisbeto
Copy link
Member

Prevents the select panel from going outside the viewport along the x axis.

Fixes #3504.
Fixes #3831.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Mar 31, 2017
@crisbeto crisbeto force-pushed the 3831/select-x-overflow branch 3 times, most recently from f9a10ab to 3393ba2 Compare April 1, 2017 16:00
@crisbeto
Copy link
Member Author

@jelbourn can you take a look at this one? I'll sort out the CI failure and the conflicts afterwards.

@jelbourn jelbourn requested a review from kara April 17, 2017 16:04
@jelbourn
Copy link
Member

@kara should look at this one, I'm not as familiar with how the select does positioning

Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

Rebase this one?

* Calculates the y-offset of the select's overlay panel in relation to the
* top start corner of the trigger. It has to be adjusted in order for the
* selected option to be aligned over the trigger when the panel opens.
*/
private _calculateOverlayOffset(selectedIndex: number, scrollBuffer: number,
private _calculateOverlayYOffset(selectedIndex: number, scrollBuffer: number,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: How about renaming to _calculateOverlayOffsetY and _calculateOverlayOffsetX? As is, it's a bit hard to scan for which one it is because the X and Y are buried in the middle.

@@ -85,6 +85,9 @@ export const SELECT_PANEL_PADDING_Y = 16;
*/
export const SELECT_PANEL_VIEWPORT_PADDING = 8;

/** Extra width that gets added the to select panel during the open animation. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: the to => to the

* content width in order to constrain the panel within the viewport.
*/
private _calculateOverlayXOffset(): void {
let overlayRect = this.overlayDir.overlayRef.overlayElement.getBoundingClientRect();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: const for the ones you don't expect to change?

@@ -85,6 +85,9 @@ export const SELECT_PANEL_PADDING_Y = 16;
*/
export const SELECT_PANEL_VIEWPORT_PADDING = 8;

/** Extra width that gets added the to select panel during the open animation. */
export const SELECT_PANEL_EXTRA_WIDTH = 32;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is always going to be twice the SELECT_PANEL_PADDING_X, no? Can we calculate this from the other constant in case it changes?


it('should stay within the viewport when overflowing on the right in rtl', async(() => {
dir.value = 'rtl';
select.style.left = '-100px';
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like either the test name or the style here is out of sync. Did you mean to say "when overflowing on the left in rtl"? The test on line 947 seems to cover the case on the right.

@kara kara assigned crisbeto and unassigned jelbourn and kara Apr 17, 2017
@crisbeto crisbeto force-pushed the 3831/select-x-overflow branch 3 times, most recently from b63f612 to 6bd9f4a Compare April 19, 2017 18:17
@crisbeto
Copy link
Member Author

Addressed the feedback, fixed the test failures, rebased and squashed @kara. Can you take one more look?

Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM

@kara kara added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Apr 20, 2017
@kara kara removed their assignment Apr 20, 2017
@kara kara added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Apr 21, 2017
@kara
Copy link
Contributor

kara commented Apr 21, 2017

@crisbeto Needs rebase

@crisbeto
Copy link
Member Author

Rebased, but it seems like one of the tests broke. Will relabel when it's fixed.

…ntally

Prevents the select panel from going outside the viewport along the x axis.

Fixes angular#3504.
Fixes angular#3831.
@crisbeto
Copy link
Member Author

There we go. Had deleted the fakeViewport.

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Apr 21, 2017
@kara kara merged commit e10bb18 into angular:master Apr 21, 2017
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mat-select is not detecting screen overflow when placeholder is hidden md-select right text overflow
4 participants