Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

feat(panel): Allow panels to be part of multiple groups. #9830

Merged
merged 1 commit into from
Oct 21, 2016
Merged

feat(panel): Allow panels to be part of multiple groups. #9830

merged 1 commit into from
Oct 21, 2016

Conversation

bradrich
Copy link
Contributor

The groupName parameter has been changed from allowing only a string to allowing a string or an array of strings. All of the necessary functionality has been changed to accompany the array.

Fixes #9565

Ping @ErinCoughlan @crisbeto and @DerekLouie for review.

crisbeto
crisbeto suggested changes Oct 13, 2016
View reviewed changes
src/components/panel/panel.js
@@ -895,7 +898,12 @@ MdPanelService.prototype.create = function(config) {
this._config.scope.$on('$destroy', angular.bind(panelRef, panelRef.detach));

if (this._config.groupName) {
panelRef.addToGroup(this._config.groupName);
if (angular.isString(this._config.groupName)) {
this._config.groupName = [this._config.groupName];
Copy link
Member

Choose a reason for hiding this comment

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

It seems like the _groupName got converted into an array. I think it's more appropriate to call it something like _groups since an array implies multiple groups.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The groupName here refers to the parameter that the user passes from the panel configuration. Since it can be either a single groupName or an array of groupNames, I was concerned about changing its name.

@ErinCoughlan what do you think about the naming convention here? Let's get the whole team's buy-in.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, sorry, I must've misread it as this._groupName.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think groupName works here.

src/components/panel/panel.js
@@ -987,7 +995,7 @@ MdPanelService.prototype.setGroupMaxOpen = function(groupName, maxOpen) {
* @private
*/
MdPanelService.prototype._openCountExceedsMaxOpen = function(groupName) {
if (groupName && this._groups[groupName]) {
if (this._groups[groupName]) {
Copy link
Member

Choose a reason for hiding this comment

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

Using indexOf here should be more appropriate. This will break with a group name of toString.

src/components/panel/panel.js
if (groupName) {
this._groups[groupName].openPanels[0].close();
}
this._groups[groupName].openPanels[0].close();
Copy link
Member

Choose a reason for hiding this comment

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

Might be appropriate to have a check here that there are any open panels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is actually only fired from the _openCountExceedsMaxOpen method above it. That method checks if the current number of open panels within the group exceeds the maximum limit and if so, the _closeFirstOpenedPanel method is called to close the first opened panel.

src/components/panel/panel.js
* for configuring the number of open panels and identifying specific
* behaviors for groups. For instance, all tooltips could be identified
* using the same groupName.
* - `groupName` - `{(string|string[])=}`: A single name of a panel group. Can
Copy link
Member

Choose a reason for hiding this comment

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

nit: You're saying "a single name of a group" and "can also be an array" in two consecutive sentences. Might be better to say something along the lines of "A group name or an array of groups".

@ThomasBurleson
Copy link
Contributor

When you ping for review, please mark as "needs: review" and "in-progress".

@ThomasBurleson ThomasBurleson added 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 14, 2016
@bradrich bradrich force-pushed the feat/panel-multiple-groups branch from 7d2ade5 to 66afca2 Compare October 14, 2016 12:59
@bradrich
Copy link
Contributor Author

@ErinCoughlan @crisbeto @DerekLouie Please review changes and provide feedback or LGTM.

crisbeto
crisbeto approved these changes Oct 14, 2016
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@bradrich bradrich force-pushed the feat/panel-multiple-groups branch from 66afca2 to 82ba80a Compare October 17, 2016 17:07
@bradrich
Copy link
Contributor Author

@ErinCoughlan Waiting on your final review for this PR.

@bradrich bradrich removed the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Oct 17, 2016
ErinCoughlan
ErinCoughlan reviewed Oct 17, 2016
View reviewed changes
src/components/panel/panel.js
* for configuring the number of open panels and identifying specific
* behaviors for groups. For instance, all tooltips could be identified
* using the same groupName.
* - `groupName` - `{(string|string[])=}`: A group name or an array of group
Copy link
Contributor

Choose a reason for hiding this comment

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

!Array

ErinCoughlan
ErinCoughlan suggested changes Oct 17, 2016
View reviewed changes
Copy link
Contributor

@ErinCoughlan ErinCoughlan left a comment

Choose a reason for hiding this comment

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

A few questions, but overall LGTM.

src/components/panel/panel.js
@@ -895,7 +898,12 @@ MdPanelService.prototype.create = function(config) {
this._config.scope.$on('$destroy', angular.bind(panelRef, panelRef.detach));

if (this._config.groupName) {
panelRef.addToGroup(this._config.groupName);
if (angular.isString(this._config.groupName)) {
this._config.groupName = [this._config.groupName];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think groupName works here.

src/components/panel/panel.spec.js
@@ -1139,18 +1164,38 @@ describe('$mdPanel', function() {
expect(getGroupPanels('test')).not.toContain(panel);
});

it('should remove a panel from a group on panel destroy', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you removing the single group tests? The config is different, so I think you still need a test or two here.

src/components/panel/panel.spec.js
expect(getGroupOpenPanels('test2')).toContain(panelRef);

closePanel();
flushPanel();
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought there was a flushPanel at the end of the closePanel. Why do you need another one?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's still here? And there's no response to the question?

src/components/panel/panel.spec.js
maxOpen: 2
});
$mdPanel.newPanelGroup('test2', {
maxOpen: 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Please choose different maxOpen for each group so we can also verify the maxes are being set of the right panels.

src/components/panel/panel.spec.js

var panel1 = $mdPanel.create(config1);
var panel2 = $mdPanel.create(config1);

Copy link
Contributor

Choose a reason for hiding this comment

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

Why the extra new line here?

@ThomasBurleson ThomasBurleson added needs: presubmit and removed needs: review This PR is waiting on review from the team labels Oct 17, 2016
@bradrich bradrich force-pushed the feat/panel-multiple-groups branch 2 times, most recently from 9c2107b to 073bcc5 Compare October 17, 2016 20:38
@bradrich
Copy link
Contributor Author

@ErinCoughlan Ping to approve your latest review.

ErinCoughlan
ErinCoughlan suggested changes Oct 17, 2016
View reviewed changes
src/components/panel/panel.spec.js
expect(getGroupOpenPanels('test2')).toContain(panelRef);

closePanel();
flushPanel();
Copy link
Contributor

Choose a reason for hiding this comment

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

It's still here? And there's no response to the question?

src/components/panel/panel.spec.js
var panel3 = $mdPanel.create(config2);
var panel4 = $mdPanel.create(config3);
var panel5 = $mdPanel.create(config3);
var panel6 = $mdPanel.create(config3);
Copy link
Contributor

Choose a reason for hiding this comment

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

This just got way too complicated. How about one group with maxOpen = 1 and one group with maxOpen =2? I don't understand why you need 6 panels to test this, even with the current values.

It would also be useful to name these related to what their config is. Below when you say panel4, I have no idea which config that is using, so it's hard to understand what you are testing and even harder to know whether it is correct.

@bradrich
feat(panel): Allow panels to be part of multiple groups.
7809bf2 
The groupName parameter has been changed from allowing only a
string to allowing a string or an array of strings. All of the
necessary functionality has been changed to accompany the array.

Fixes angular#9565
@bradrich bradrich force-pushed the feat/panel-multiple-groups branch from 073bcc5 to 7809bf2 Compare October 18, 2016 12:13
@bradrich
Copy link
Contributor Author

@ErinCoughlan Latest ping!

ErinCoughlan
ErinCoughlan approved these changes Oct 18, 2016
View reviewed changes
Copy link
Contributor

@ErinCoughlan ErinCoughlan left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM

@bradrich
Copy link
Contributor Author

@ThomasBurleson Final LGTM has been given.

@jelbourn jelbourn merged commit 80e87b5 into angular:master Oct 21, 2016
@Splaktar Splaktar added pr: merge ready This PR is ready for a caretaker to review resolution: fixed labels Mar 7, 2020
@Splaktar Splaktar added this to the 1.1.2 milestone Mar 7, 2020
@angular angular locked as resolved and limited conversation to collaborators Mar 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
2 more reviewers

@ErinCoughlan ErinCoughlan ErinCoughlan approved these changes

@crisbeto crisbeto crisbeto approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@crisbeto crisbeto

Labels
pr: merge ready This PR is ready for a caretaker to review
Projects
None yet
Milestone
1.1.2
Development

Successfully merging this pull request may close these issues.

mdPanel: Allow panels to be part of multiple groups.
7 participants
@bradrich @ThomasBurleson @ErinCoughlan @crisbeto @DerekLouie @jelbourn @Splaktar