feat(panel): Configuration ID for tracking #9379
Conversation
| * @param {!Object=} config Configuration object for the panel. | ||
| * @returns {!MdPanelRef} | ||
| */ | ||
| MdPanelService.prototype.create = function(config) { | ||
| var configSettings = config || {}; | ||
|
|
||
| if (angular.isDefined(configSettings.id) && |
There was a problem hiding this comment.
I can understand why angular.isDefined(configSettings.id) is necessary but
angular.isDefined(this._trackedPanels[configSettings.id]) is pretty much this._trackedPanels[configSettings.id]
correct me if i'm wrong
There was a problem hiding this comment.
@EladBezalel You are correct about that. Is the angular.isDefined() method more expensive?
There was a problem hiding this comment.
Not sure, just makes more sense to me :)
There was a problem hiding this comment.
@EladBezalel I don't see why not! I will change it and commit.
bradrich
force-pushed
the
feat/panel-config-id
branch
from
24ac2f6 to
1c08e5e
Compare
| * Creates a panel with the specified options. | ||
| * | ||
| * @param {!Object=} config Configuration object for the panel. | ||
| * @returns {!MdPanelRef} | ||
| */ | ||
| MdPanelService.prototype.create = function(config) { |
There was a problem hiding this comment.
@EladBezalel I think that this is a much better formatted create method. Do you think so?
ThomasBurleson
added
the
pr: merge ready
| @@ -637,7 +660,9 @@ var FOCUS_TRAP_TEMPLATE = angular.element( | |||
|
|
|||
|
|
|||
| /** | |||
| * @description | |||
There was a problem hiding this comment.
Where do the @decription come from? These aren't ngdoc, like above where @decription matters.
There was a problem hiding this comment.
@ErinCoughlan I just thought that all documentation / comment areas should follow the same format. I will remove the @description tags.
|
Is this still work in progress? It is currently married for merging |
|
@ThomasBurleson I am finalizing the nits that @ErinCoughlan has assigned. Please wait for a LGTM from her. |
bradrich
force-pushed
the
feat/panel-config-id
branch
from
1c08e5e to
7b7d7d4
Compare
|
@ErinCoughlan All nits have been corrected. Please re-review and provide your LGTM. |
ThomasBurleson
added
in progress
| @@ -1320,6 +1377,7 @@ MdPanelRef.prototype._updatePosition = function(init) { | |||
|
|
|||
| /** | |||
| * Focuses on the panel or the first focus target. | |||
| * | |||
There was a problem hiding this comment.
So a lot of this extra space feels excessive, especially for methods like this one, where the description is only a single line and there's no extra information we care about. I've never seen it before (unless it's a multi-paragraph description).
Where does this pattern come from?
Here's the style guide we follow:
https://google.github.io/styleguide/javascriptguide.xml?showone=Comments#Comments
| @@ -2090,7 +2197,8 @@ MdPanelPosition.prototype._setPanelPosition = function(panelEl) { | |||
|
|
|||
|
|
|||
| /** | |||
| * Switching between 'start' and 'end' | |||
| * Switching between 'start' and 'end'. | |||
There was a problem hiding this comment.
Switching -> Switches
bradrich
force-pushed
the
feat/panel-config-id
branch
from
7b7d7d4 to
b6ee88b
Compare
|
@ErinCoughlan Round two... FIGHT! |
|
LGTM, looks like you need to rebase again. There are conflicts with this branch. |
|
Interesting. I will handle this before EOD today. |
The user is now able to provide an ID as a property of the panel configuration object. When an ID is provided, the created panel is added to a tracked panels object. Any subsequent requests made to create a panel with that ID are ignored. This is useful in having the panel service not open multiple panels from the same user interaction when there is no backdrop and events are propagated. Fixes angular#9356 Also, several syntax issues throughout the documentation/commenting have been corrected. Fixes angular#9357 Ping ErinCoughlan
bradrich
force-pushed
the
feat/panel-config-id
branch
from
b6ee88b to
bcbc2ab
Compare
ThomasBurleson
added
pr: merge ready
There was a problem hiding this comment.
@ErinCoughlan @bradrich @ThomasBurleson I think we need to adjust the logic here to make it work properly. Anyone of you still spending time on this repo? I could make pull-requests^^ But if i dont know if they getting merged (like other PR atm) i dont want to invest time...
| var instanceId = 'panel_' + this._$injector.get('$mdUtil').nextUid(); | ||
| var instanceConfig = angular.extend({ id: instanceId }, this._config); | ||
| var panelRef = new MdPanelRef(this._config, this._$injector); | ||
| this._trackedPanels[config.id] = panelRef; |
There was a problem hiding this comment.
config.id is undefined if no id is set!
There was a problem hiding this comment.
this._trackedPanels is set here but there is no chance of removing elements here.
Example: Create config with id-property and create panel. So in the first run a new scope is set and the panelRef is cached in _trackedPanels. Now destroy() the panelRef and create a new panel-instance with the same id -> the scope is $destroy(ed) but the old panelRef with the destroyed scope is used.
|
@IMM0rtalis we're addressing the hold ATM, creating a pr would help us, as soon as we will resolve our internal issues with merging PRs we will merge your PRs and will try to revive the project. |
The user is now able to provide an ID as a property of the panel configuration object. When an ID is provided, the created panel is added to a tracked panels object. Any subsequent requests made to create a panel with that ID are ignored. This is useful in having the panel service not open multiple panels from the same user interaction when there is no backdrop and events are propagated.
Fixes #9356
Also, several syntax issues throughout the documentation/commenting have been corrected.
Fixes #9357
Ping ErinCoughlan