update(panel): constrain panel to viewport boundries #9651

crisbeto · 2016-09-20T20:57:21Z

Prevents the panel from going outside the viewport by adjusting the position.
If developers want more control over how the panel gets repositioned, they can specify addition fallback positions via addPanelPosition.

Related to #9641.

Fixes #7878.

ErinCoughlan · 2016-09-21T00:21:08Z

src/components/panel/panel.js

+   * Margin between the edges of the panel and the viewport
+   * @type {number}
+   */
+  this.viewportMargin = MdPanelPosition.viewportMargin;


I don't think this one needs to be re-exported via $mdPanel, since there are no values for people to access or change.

The main reason was to expose it to the unit tests. It can also be a global variable there that can be easily changed if tests start failing because of the wrong margin.

ErinCoughlan · 2016-09-21T00:21:23Z

src/components/panel/panel.js

@@ -1767,6 +1773,12 @@ MdPanelPosition.absPosition = {
  LEFT: 'left'
 };

+/**
+ * Margin between the edges of a panel and the viewport.
+ * @type {number}


Should this use @const?

ErinCoughlan · 2016-09-21T00:23:29Z

src/components/panel/panel.js

+ * Margin between the edges of a panel and the viewport.
+ * @type {number}
+ */
+MdPanelPosition.viewportMargin = 5;


5 -> 8 (everything in Material design is based on 8s.)

https://material.google.com/layout/responsive-ui.html#responsive-ui-grid

crisbeto · 2016-09-21T16:40:47Z

Included the feedback from Erin.

ErinCoughlan

LGTM

Prevents the panel from going outside the viewport by adjusting the position. If developers want more control over how the panel gets repositioned, they can specify addition fallback positions via `addPanelPosition`. Related to angular#9641. Fixes angular#7878.

crisbeto · 2016-09-26T23:20:11Z

@ThomasBurleson this should be ready for presubmit.

henrymana · 2016-10-11T23:24:43Z

src/components/panel/panel.js

+    }
+  }
+
+  if (this.getLeft()) {


There is one case when the panel width is 100% in which there is no right side margin. Should the intent of this issue cover that scenario as well? Or is that something to handle as a separate issue or by the user of the md-panel? I am able to adjust using "withXPosition('align-left')"... but I am just wondering if you think this should be performed by the constrainToViewport method.

Good question. IMO the constrainToViewport method is a last-ditch effort to keep the panel in the viewport and it may not be successful (e.g. the element is wider than the viewport). Before mdPanel resorts to it, it tries all of the available positions to see which one stays in the viewport. If you want a bit more control, you can add extra positions via addPanelPosition and it'll try those first. Here's an example:

$mdPanel.newPanelPosition() .relativeTo('something') .addPanelPosition(this._mdPanel.xPosition.ALIGN_START, this._mdPanel.yPosition.BELOW) .addPanelPosition(this._mdPanel.xPosition.ALIGN_END, this._mdPanel.yPosition.BELOW) .addPanelPosition(this._mdPanel.xPosition.ALIGN_START, this._mdPanel.yPosition.ABOVE);

This will try to position it start/below, end/below, start/above before resorting to constraining it.

Also I think you asked yesterday about the "position adjusted" class. This is useful in the cases where the panel was constrained, because it allows you to do some specific styling (e.g. we use it in the datepicker to hide a part of the overlay).

I do use multiple positions. The panel does not really go off the screen, but it touches the right edge of the viewport with no margin (so I set a negative offsetX). Thinking about it more, that could probably be handled by css instead. Thank you for this change. I am already trying out your changes.

henrymana · 2016-10-12T20:03:03Z

I noticed earlier that the md-panel position is not updated when disableParentScroll is set to true (and as a result the panel falls outside the viewport boundaries). In my case the md-panel is at the bottom of the screen and this results in part of the panel being offscreen.

The panel is positioned correctly within the viewport when disableParentScroll is set to false.

Looking at the code, I can see this was a pre-existing behavior. I haven't finished debugging but the issue may have to do with the positionUpdate performed when the onScroll event handler is added in line 1783 ( this._$window.addEventListener('scroll', onScroll, true) ).

Should re-positioning work when the parent scroll is disabled? Should I open a separate issue to handle this?

crisbeto · 2016-10-12T20:14:27Z

I'm not sure about it, but it would be great if you made an issue so it's easier to track and discuss. It does sound strange though.

henrymana · 2016-10-12T20:17:46Z

Ok, I will create a small test to replicate and create a new issue later in the week. Thank you.

henrymana · 2016-10-13T18:51:08Z

src/components/panel/panel.js

+MdPanelPosition.prototype._constrainToViewport = function(panelEl) {
+  var margin = MdPanelPosition.viewportMargin;
+
+  if (this.getTop()) {


The md-datepicker code has a comment regarding "disabled body scrolling" which I think is applicable here as well. I need to understand the code more, but I think this may be a gap in the constrainToViewport implementation.

Comment from: https://github.com/angular/material/pull/9641/files#diff-31bf126782bc06608d149fe72237bf4eL640

If ng-material has disabled body scrolling (for example, if a dialog is open), then it's possible that the already-scrolled body has a negative top/left. In this case, we want to treat the "real" top as (0 - bodyRect.top). In a normal scrolling situation, though, the top of the viewport should just be the body's scroll position.

I doubt it, because the panel is always positioned relatively to the viewport (position: fixed). The current datepicker implementation positions the calendar relatively to the body (via position: absolute).

ok, thank you.

henrymana · 2016-10-13T19:18:52Z

I am no longer able to replicate the issue I mentioned earlier with disableParentScroll (I had a fresh checkout of the master branch and your branch only.). I am not sure why it failed earlier but it seems to be working now. Thank you again.

crisbeto self-assigned this Sep 20, 2016

crisbeto added the needs: work label Sep 20, 2016

crisbeto force-pushed the 7878/panel-constrain branch 2 times, most recently from 2505bd8 to 1284aa9 Compare September 20, 2016 21:22

crisbeto added needs: review This PR is waiting on review from the team and removed needs: work labels Sep 20, 2016

crisbeto assigned ThomasBurleson, ErinCoughlan and bradrich and unassigned crisbeto Sep 20, 2016

ErinCoughlan reviewed Sep 21, 2016

View reviewed changes

ThomasBurleson added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs and removed needs: review This PR is waiting on review from the team labels Sep 21, 2016

ThomasBurleson added this to the 1.1.2 milestone Sep 21, 2016

crisbeto added the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Sep 21, 2016

crisbeto force-pushed the 7878/panel-constrain branch from 1284aa9 to d80b0b8 Compare September 21, 2016 16:40

crisbeto removed the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Sep 21, 2016

crisbeto mentioned this pull request Sep 21, 2016

feat(datepicker): migrate to mdPanel #9641

Open

3 tasks

ErinCoughlan approved these changes Sep 21, 2016

View reviewed changes

crisbeto added the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Sep 26, 2016

crisbeto force-pushed the 7878/panel-constrain branch from d80b0b8 to fa5c22e Compare September 26, 2016 23:19

crisbeto added needs: review This PR is waiting on review from the team and removed needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved labels Sep 26, 2016

ThomasBurleson added needs: presubmit and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: review This PR is waiting on review from the team labels Oct 10, 2016

henrymana reviewed Oct 11, 2016

View reviewed changes

henrymana reviewed Oct 13, 2016

View reviewed changes

kara added pr: merge ready This PR is ready for a caretaker to review and removed needs: presubmit labels Oct 16, 2016

kara merged commit d464904 into angular:master Oct 16, 2016

crisbeto mentioned this pull request Oct 16, 2016

datepicker: migrate to use mdPanel container #9564

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update(panel): constrain panel to viewport boundries #9651

update(panel): constrain panel to viewport boundries #9651

crisbeto commented Sep 20, 2016

ErinCoughlan Sep 21, 2016

crisbeto Sep 21, 2016

ErinCoughlan Sep 21, 2016 •

edited

ErinCoughlan Sep 21, 2016

crisbeto commented Sep 21, 2016

ErinCoughlan left a comment

crisbeto commented Sep 26, 2016

henrymana Oct 11, 2016 •

edited

crisbeto Oct 12, 2016 •

edited

henrymana Oct 12, 2016

henrymana commented Oct 12, 2016

crisbeto commented Oct 12, 2016

henrymana commented Oct 12, 2016

henrymana Oct 13, 2016

crisbeto Oct 13, 2016

henrymana Oct 13, 2016

henrymana commented Oct 13, 2016

update(panel): constrain panel to viewport boundries #9651

update(panel): constrain panel to viewport boundries #9651

Conversation

crisbeto commented Sep 20, 2016

ErinCoughlan Sep 21, 2016

Choose a reason for hiding this comment

crisbeto Sep 21, 2016

Choose a reason for hiding this comment

ErinCoughlan Sep 21, 2016 • edited

Choose a reason for hiding this comment

ErinCoughlan Sep 21, 2016

Choose a reason for hiding this comment

crisbeto commented Sep 21, 2016

ErinCoughlan left a comment

Choose a reason for hiding this comment

crisbeto commented Sep 26, 2016

henrymana Oct 11, 2016 • edited

Choose a reason for hiding this comment

crisbeto Oct 12, 2016 • edited

Choose a reason for hiding this comment

henrymana Oct 12, 2016

Choose a reason for hiding this comment

henrymana commented Oct 12, 2016

crisbeto commented Oct 12, 2016

henrymana commented Oct 12, 2016

henrymana Oct 13, 2016

Choose a reason for hiding this comment

crisbeto Oct 13, 2016

Choose a reason for hiding this comment

henrymana Oct 13, 2016

Choose a reason for hiding this comment

henrymana commented Oct 13, 2016

ErinCoughlan Sep 21, 2016 •

edited

henrymana Oct 11, 2016 •

edited

crisbeto Oct 12, 2016 •

edited