This repository has been archived by the owner on Aug 29, 2023. It is now read-only.

feat(datepicker): migrate to mdPanel #9641

Open

crisbeto wants to merge 1 commit into angular:master from crisbeto:9564/datepicker-panel

Member

crisbeto commented Sep 19, 2016 •

edited by Splaktar

Migrates the datepicker's positioning logic to mdPanel. All the functionality should work as before.

Fixes #9564.

Notes

This PR technically works and it is ready for code review, however in order to achieve feature parity with the current datepicker we need the following:

Merging in PR update(panel): constrain panel to viewport boundries #9651 that will prevent the calendar from going off-screen.
Merging in feat(panel): allow passing in a function to the offset methods #9615. I've been developing this PR with the changes from feat(panel): allow passing in a function to the offset methods #9615 already in place and can confirm that it works.
Being able to animate a panel's transform when it has offsets (tracking via feat(panel): allow transform to be animated on elements with an offset #9905).

crisbeto added needs: review Blocked labels

crisbeto assigned ThomasBurleson, ErinCoughlan and bradrich

crisbeto force-pushed the 9564/datepicker-panel branch 8 times, most recently from 5b88abe to 3f6568c Compare

September 20, 2016 16:16

crisbeto added a commit to crisbeto/material that referenced this pull request


          update(panel): constrain panel to viewport boundries

22bb648

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 mentioned this pull request

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

Merged

crisbeto added a commit to crisbeto/material that referenced this pull request


          update(panel): constrain panel to viewport boundries

2505bd8

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 added a commit to crisbeto/material that referenced this pull request


          update(panel): constrain panel to viewport boundries

1284aa9

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.

ThomasBurleson added in progress and removed needs: review labels

crisbeto added a commit to crisbeto/material that referenced this pull request


          update(panel): constrain panel to viewport boundries

d80b0b8

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 force-pushed the 9564/datepicker-panel branch from 3f6568c to b5f7cdc Compare

September 21, 2016 16:43

ErinCoughlan reviewed

View reviewed changes

src/components/datepicker/js/calendar.js

+                  if (this._panelRef) {
+                    // Bind the keydown handler to the body, in order to handle cases where the focused
+                    // element gets removed from the DOM and stops propagating key events.
+                    angular.element(document.body).on('keydown', boundKeyHandler);

Contributor

ErinCoughlan Sep 21, 2016

This is my first time looking at the date picker code. Can you explain what edge case this helps to prevent? Do other components care about this or have similar functionality?

Member Author

crisbeto Sep 22, 2016

The calendar currently focuses the active date's cell (I believe for accessibility reasons), which causes any keyboard events to be captured by the calendar. The issue is that the calendar uses mdVirtualRepeat which will remove the cell eventually, if you scroll long enough. This causes the browser to shift focus back to the body which stops the keyboard events from working.

src/components/datepicker/js/datepickerDirective.js

-                  /** @type {boolean} Whether the date-picker's calendar pane is open. */
+                  /** @type {boolean} Whether the date-picker's calendar pane is open. Used internally. */
                   this.isCalendarOpen = false;

Contributor

ErinCoughlan Sep 22, 2016

Used internally, therefore should be marked private?

src/components/datepicker/js/datepickerDirective.js

+                  this.isOpen = false;
+                  /** @type {boolean} Used to prevent infinite loops when using mdOpenOnFocus. */
+                  this.preventInputFocus = false;

Contributor

ErinCoughlan Sep 22, 2016

If you're using panels focusOnOpen, is this needed?

Member Author

crisbeto Sep 22, 2016

Yes, this is also a private property. When the calendar closes, it restores focus to the element that triggered it to open. With mdOpenOnFocus that would cause the calendar to open up again since it refocuses the input. We use this flag to prevent such cases. It used to be possible to do this by delaying the close handler, but it got a little trickier with mdPanel since it's also async.

src/components/datepicker/js/datepickerDirective.js

+                    clickOutsideToClose: true,
+                    escapeToClose: true,
+                    focusOnOpen: false,
+                    scope: this.$scope.$new(),

Contributor

ErinCoughlan Sep 22, 2016

I think this is the default, so you can remove it if you want. Being explicit is also fine though. Up to you.

Member Author

crisbeto Sep 22, 2016

The default creates a scope that's a child of $rootScope, however we need it to be a child of the datepicker's scope.

src/components/datepicker/js/datepickerDirective.js

-                    width: (elementRect.width - 1) + 'px',
-                    height: (elementRect.height - 2) + 'px'
-                  });
+                  var self = this;

Contributor

ErinCoughlan Sep 22, 2016

So much code goes bye-bye. This is amazing!

src/components/datepicker/js/datepickerDirective.js

-                  calendarPane.style.left = paneLeft + 'px';
-                  calendarPane.style.top = paneTop + 'px';
-                  document.body.appendChild(calendarPane);
+                      // TODO(crisbeto): this should use getPanelYOffset once we ditch the units

Contributor

ErinCoughlan Sep 22, 2016

Can this be done now?

Member Author

crisbeto Sep 22, 2016

It's waiting on #9609 to be merged.

crisbeto added the needs: rebase label

crisbeto added a commit to crisbeto/material that referenced this pull request


          update(panel): constrain panel to viewport boundries

fa5c22e

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 force-pushed the 9564/datepicker-panel branch from b5f7cdc to 0362caa Compare

September 26, 2016 23:24

crisbeto removed the needs: rebase label

henrymana reviewed

View reviewed changes

src/components/datepicker/js/datepickerDirective.js

+                    .addPanelPosition($mdPanel.xPosition.ALIGN_START, $mdPanel.yPosition.ALIGN_TOPS)
+                    .withOffsetX(-this.leftMargin + 'px')
+                    .withOffsetY(angular.bind(this, this.getPanelYOffset));

henrymana Oct 3, 2016

The current version of the md-datepicker hides the popup when the window is re-sized. I noticed that the md-panel stays in the original position when the window is resized and using "relativeTo" (it stops being relativeTo the original element).

Can you confirm what the behavior is for the new implementation?

Member Author

crisbeto Oct 4, 2016

It will still hide on resize (see https://github.com/angular/material/pull/9641/files#diff-31bf126782bc06608d149fe72237bf4eR702), although now it might be a little more practical to update the position instead. mdPanel has a convenient updatePosition method.

henrymana Oct 5, 2016

Cool, I see it know... (window.addEventListener(this.windowEventName, this.windowEventHandler) ). Thank you.

crisbeto mentioned this pull request

calendar: doesn't support tabbing out of it when used standalone #9794

Closed

crisbeto force-pushed the 9564/datepicker-panel branch from 0362caa to 50f2638 Compare

October 14, 2016 18:55

kara pushed a commit that referenced this pull request


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

d464904

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.

crisbeto added a commit to crisbeto/material that referenced this pull request


          fix(panel): allow transform to be animated on an offset panel

c9f01b7

Previously, if a panel had a position with an offset (e.g. `$mdPanel.newPanelPosition().center()`), the positioning would break any `transform` animations on the panel. This was due to the fact that `mdPanel` uses inline `transform` styles to do the offsetting. These changes introduce a wrapper around the panel (`.md-panel-inner-wrapper`), which will handle all of the positioning, allowing for any animations to be applied to the `.md-panel` itself.

Relates to angular#9641.
Fixes angular#9905.

crisbeto added a commit to crisbeto/material that referenced this pull request


          fix(panel): allow transform to be animated on an offset panel

ae4bdca

Previously, if a panel had a position with an offset (e.g. `$mdPanel.newPanelPosition().center()`), the positioning would break any `transform` animations on the panel. This was due to the fact that `mdPanel` uses inline `transform` styles to do the offsetting. These changes introduce a wrapper around the panel (`.md-panel-inner-wrapper`), which will handle all of the positioning, allowing for any animations to be applied to the `.md-panel` itself.

Relates to angular#9641.
Fixes angular#9905.

ThomasBurleson modified the milestones: 1.1.3, 1.2.0

crisbeto added a commit to crisbeto/material that referenced this pull request


          fix(panel): allow transform to be animated on an offset panel

ce1f5c9

Previously, if a panel had a position with an offset (e.g. `$mdPanel.newPanelPosition().center()`), the positioning would break any `transform` animations on the panel. This was due to the fact that `mdPanel` uses inline `transform` styles to do the offsetting. These changes introduce a wrapper around the panel (`.md-panel-inner-wrapper`), which will handle all of the positioning, allowing for any animations to be applied to the `.md-panel` itself.

Relates to angular#9641.
Fixes angular#9905.

Splaktar added P2: required a11y needs: rebase labels

Splaktar assigned Splaktar and unassigned ThomasBurleson and bradrich

crisbeto added a commit to crisbeto/material that referenced this pull request


          fix(panel): allow transform to be animated on an offset panel

9cad06e

Previously, if a panel had a position with an offset (e.g. `$mdPanel.newPanelPosition().center()`), the positioning would break any `transform` animations on the panel. This was due to the fact that `mdPanel` uses inline `transform` styles to do the offsetting. These changes introduce a wrapper around the panel (`.md-panel-inner-wrapper`), which will handle all of the positioning, allowing for any animations to be applied to the `.md-panel` itself.

Relates to angular#9641.
Fixes angular#9905.

Splaktar mentioned this pull request

fix(panel): allow transform to be animated on an offset panel #11389

Closed

3 tasks

Splaktar pushed a commit that referenced this pull request


          fix(panel): allow transform to be animated on an offset panel

93d6572

Previously, if a panel had a position with an offset (e.g. `$mdPanel.newPanelPosition().center()`), the positioning would break any `transform` animations on the panel. This was due to the fact that `mdPanel` uses inline `transform` styles to do the offsetting. These changes introduce a wrapper around the panel (`.md-panel-inner-wrapper`), which will handle all of the positioning, allowing for any animations to be applied to the `.md-panel` itself.

Relates to #9641.
Fixes #9905.

Splaktar mentioned this pull request

feat(panel): allow transform to be animated on an offset panel #11390

Merged

6 tasks

Splaktar unassigned ErinCoughlan

Splaktar modified the milestones: Future, 1.2.0

Splaktar added g3: reported hotlist: animations labels

Splaktar added this to In-Progress in Panel Refactors

Splaktar pushed a commit that referenced this pull request


          fix(panel): allow transform to be animated on an offset panel

7ef555e

Previously, if a panel had a position with an offset (e.g. `$mdPanel.newPanelPosition().center()`), the positioning would break any `transform` animations on the panel. This was due to the fact that `mdPanel` uses inline `transform` styles to do the offsetting. These changes introduce a wrapper around the panel (`.md-panel-inner-wrapper`), which will handle all of the positioning, allowing for any animations to be applied to the `.md-panel` itself.

Relates to #9641.
Fixes #9905.

Splaktar pushed a commit that referenced this pull request


          fix(panel): allow transform to be animated on an offset panel

c3a30fb

Previously, if a panel had a position with an offset (e.g. `$mdPanel.newPanelPosition().center()`), the positioning would break any `transform` animations on the panel. This was due to the fact that `mdPanel` uses inline `transform` styles to do the offsetting. These changes introduce a wrapper around the panel (`.md-panel-inner-wrapper`), which will handle all of the positioning, allowing for any animations to be applied to the `.md-panel` itself.

Relates to #9641.
Fixes #9905.

Splaktar pushed a commit that referenced this pull request


          fix(panel): allow transform to be animated on an offset panel

269b68e

Previously, if a panel had a position with an offset (e.g. `$mdPanel.newPanelPosition().center()`), the positioning would break any `transform` animations on the panel. This was due to the fact that `mdPanel` uses inline `transform` styles to do the offsetting. These changes introduce a wrapper around the panel (`.md-panel-inner-wrapper`), which will handle all of the positioning, allowing for any animations to be applied to the `.md-panel` itself.

Relates to #9641.
Fixes #9905.

Splaktar removed the Blocked label

Splaktar modified the milestones: 1.2.0, 1.3.0

Splaktar moved this from In-Progress to Rejected in Panel Refactors

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