fix(mdPanel): Propagation, CSS targeting, and dynamic position updating #8983

bradrich · 2016-07-08T20:32:30Z

Fixes #8968 by:

Adding the propagateContainerEvents option to the panel configuration that is defaulted to false. If true, the mdPanel API will allow all touch and pointer events to propagate through the mdPanel container, the 'outer-wrapper', to the elements behind it.

Fixes #8980 by:

Adding a boolean parameter to the MdPanelRef addClass, removeClass, and toggleClass functions that defaults to false. When true, it will allow the function's primary class actions to be executed on the mdPanel element, instead of the container.
Adding a public updatePosition function that will take in a MdPanelPosition object parameter that it will overwrite the current configuration's position and then call the private _updatePosition function.

ErinCoughlan · 2016-07-08T20:53:01Z

src/components/panel/panel.js

@@ -1188,6 +1211,24 @@ MdPanelRef.prototype._addStyles = function() {


 /**
+ * Update the position configuration of a panel


nit: Method descriptions should start with a sentence written in the third person declarative voice. Can you change this to:

"Updates the position configuration of the panel."

bradrich · 2016-07-08T21:23:25Z

@ErinCoughlan Thank you for these comments. I will make your suggested changes.

EladBezalel · 2016-07-09T00:08:41Z

IMO we should remove addClass, removeClass and toggleClass by providing the container and the panel elements as a public api.
Doesn't makes sense copying this functionality from jqlite.
@ErinCoughlan

ErinCoughlan · 2016-07-11T19:08:35Z

@EladBezalel That's fine by me.

bradrich · 2016-07-11T20:35:50Z

@topherfangio Squashed my commits on this, even though it needs work.

topherfangio · 2016-07-12T17:09:50Z

@bradrich Awesome; thanks! In general, if you push anything to a PR (even in-progress), you should squash your commits 😄

ErinCoughlan · 2016-07-14T18:46:46Z

@bradrich Tests? You added new functionality, but I have no idea if they work and will continue to work.

bradrich · 2016-07-15T13:00:26Z

@ErinCoughlan All tests are included and passing. Please review and give LGTM.

Ping @ThomasBurleson

bradrich · 2016-07-15T13:01:03Z

src/components/panel/panel.spec.js

@@ -1115,6 +1212,63 @@ describe('$mdPanel', function() {
      mdPanelPosition = $mdPanel.newPanelPosition();
    });

+    it('should update the position of an open panel', function() {


@ErinCoughlan Please give special attention to this test.

ErinCoughlan · 2016-07-15T16:03:05Z

src/components/panel/panel.spec.js

+    describe('CSS class logic:', function() {
+      it('should add a class to the container/wrapper when toElement=false',
+        function() {
+        openPanel(DEFAULT_CONFIG);


nit: indentation here is off. openPanel and the rest of the code still needs to be indented under function, like this:

it('should add a class to the container/wrapper when toElement=false', function() { openPanel(DEFAULT_CONFIG); ... expect(PANEL_WRAPPER_CLASS).toHaveCLASS('my-class'); });

bradrich · 2016-07-15T16:18:50Z

@ErinCoughlan That's it! Whew!

EDIT: Ah, I spoke too soon.

ErinCoughlan · 2016-07-15T17:16:36Z

src/components/panel/panel.js

+  var self = this;
+
+  self._config['position'] = position;
+  self._$mdUtil.nextTick(function() {


Does this work?

self._$mdUtil.nextTick(self._updatePosition);

I guess it would work if you bind it to this, since updatePosition accessed the instance context?

It does not. I have new tests coming in.

bradrich · 2016-07-15T19:44:57Z

@EladBezalel I am going to add the panel wrapper/container and element to the panelRef in another feature. So, I am going to remove the needs: work label.

bradrich · 2016-07-15T19:45:35Z

@ThomasBurleson There is an issue with the Travis CI. Is it known?

devversion · 2016-07-15T19:48:02Z

@bradrich This rarely happens to builds on Saucelabs. I will try to increase the Sauce timeout, so we reduce that flakiness a bit.

ErinCoughlan · 2016-07-15T20:38:04Z

src/components/panel/panel.js

+    throw new Error('Panel does not exist yet. Call open() or attach().');
+  }
+
+  var self = this;


I think the self variable here is no longer necessary and you can use this for both lines below.

ErinCoughlan · 2016-07-15T20:48:40Z

@ThomasBurleson @bradrich LGTM once nits are resolved

@ErinCoughlan

Fixes angular#8968 by: - Adding the `propagateContainerEvents` option to the panel configuration that is defaulted to false. If true, the mdPanel API will allow all touch and pointer events to propagate through the mdPanel container, the 'outer-wrapper', to the elements behind it. Fixes angular#8980 by: - Adding a boolean parameter to the MdPanelRef `addClass`, `removeClass`, and `toggleClass` functions that defaults to false. When true, it will allow the function's primary class actions to be executed on the mdPanel element, instead of the container. - Adding a public `updatePosition` function that will take in a MdPanelPosition object parameter that it will overwrite the current configuration's position and then call the private `_updatePosition` function. Ping @ErinCoughlan

bradrich · 2016-07-15T21:44:58Z

@ErinCoughlan & @ThomasBurleson All requests have been met and everything should be GTG. Please let me know if I have missed anything.

Thanks for everyone's attention.

bradrich assigned ThomasBurleson, ErinCoughlan and EladBezalel Jul 8, 2016

bradrich mentioned this pull request Jul 8, 2016

feat(tooltip): Tooltip transition to mdPanel #8984

Closed

ErinCoughlan reviewed Jul 8, 2016
View reviewed changes

EladBezalel added the needs: work label Jul 9, 2016

bradrich assigned topherfangio Jul 11, 2016

bradrich force-pushed the fix/panel-propagation-css-position branch from ea65c7c to 32bd19c Compare July 11, 2016 20:34

bradrich force-pushed the fix/panel-propagation-css-position branch from 32bd19c to 6148b5e Compare July 15, 2016 12:58

bradrich reviewed Jul 15, 2016
View reviewed changes

bradrich force-pushed the fix/panel-propagation-css-position branch 2 times, most recently from c38d063 to bdc9fd1 Compare July 15, 2016 13:06

ErinCoughlan reviewed Jul 15, 2016
View reviewed changes

bradrich force-pushed the fix/panel-propagation-css-position branch from bdc9fd1 to 5bb2e23 Compare July 15, 2016 19:31

bradrich removed the needs: work label Jul 15, 2016

ErinCoughlan reviewed Jul 15, 2016
View reviewed changes

bradrich force-pushed the fix/panel-propagation-css-position branch from 5bb2e23 to 127dd54 Compare July 15, 2016 21:42

ThomasBurleson closed this in 4efafcf Jul 16, 2016

bradrich deleted the fix/panel-propagation-css-position branch July 21, 2016 23:34

		@@ -1188,6 +1211,24 @@ MdPanelRef.prototype._addStyles = function() {


		/**
		* Update the position configuration of a panel

fix(mdPanel): Propagation, CSS targeting, and dynamic position updating #8983

fix(mdPanel): Propagation, CSS targeting, and dynamic position updating #8983

Uh oh!

Conversation

bradrich commented Jul 8, 2016

Uh oh!

ErinCoughlan Jul 8, 2016

Choose a reason for hiding this comment

Uh oh!

bradrich commented Jul 8, 2016

Uh oh!

EladBezalel commented Jul 9, 2016

Uh oh!

ErinCoughlan commented Jul 11, 2016

Uh oh!

bradrich commented Jul 11, 2016

Uh oh!

topherfangio commented Jul 12, 2016

Uh oh!

ErinCoughlan commented Jul 14, 2016

Uh oh!

bradrich commented Jul 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bradrich Jul 15, 2016

Choose a reason for hiding this comment

Uh oh!

ErinCoughlan Jul 15, 2016

Choose a reason for hiding this comment

Uh oh!

bradrich commented Jul 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ErinCoughlan Jul 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devversion Jul 15, 2016

Choose a reason for hiding this comment

Uh oh!

bradrich Jul 15, 2016

Choose a reason for hiding this comment

Uh oh!

bradrich commented Jul 15, 2016

Uh oh!

bradrich commented Jul 15, 2016

Uh oh!

devversion commented Jul 15, 2016

Uh oh!

ErinCoughlan Jul 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErinCoughlan commented Jul 15, 2016

Uh oh!

bradrich commented Jul 15, 2016

Uh oh!

Uh oh!

bradrich commented Jul 15, 2016 •

edited

Loading

bradrich commented Jul 15, 2016 •

edited

Loading

ErinCoughlan Jul 15, 2016 •

edited

Loading

ErinCoughlan Jul 15, 2016 •

edited

Loading