Skip to content

Conversation

jeff-phillips-18
Copy link
Member

This addresses issue #306

@dgutride @dtaylor113 @AparnaKarve

@dtaylor113
Copy link
Member

Looks good. Any unit test coverage for these new options?

@jeff-phillips-18
Copy link
Member Author

@dtaylor113 Added unit tests for these options

@@ -238,6 +242,18 @@
},
];

$scope.getMenuClass = function (item) {
var menuClass = "";
if (item.name === "Jim Beam") {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a slightly less risque name here? Not sure if we have public standards for documentation or if I'm just being overly cautious...

Copy link
Member

Choose a reason for hiding this comment

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

If we use it all over the place, let's leave it - I wasn't sure if the reference would be noticed by some given the connotation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed the name to protect the innocent 👮

@jeff-phillips-18
Copy link
Member Author

Heh, that name has been used here since its inception. Do you think I should change it?

@dgutride
Copy link
Member

I'm good with this.

@AparnaKarve
Copy link

Thanks for this PR @jeff-phillips-18
I haven't tested this yet, but I'm A-OK with @dtaylor113 and @dgutride 's review 👍

@dtaylor113 dtaylor113 merged commit aadf359 into patternfly:master Oct 12, 2016
@AparnaKarve
Copy link

Quick question: To be able to test these changes with SSUI, do I have to wait for the next version of angular-patternfly ? Currently SSUI uses "angular-patternfly": "^3.11.0"

@jeff-phillips-18
Copy link
Member Author

Yes, angular patternfly should release 3.12.0 either today or very shortly and this PR looks to be included.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants