Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(pfNotificationDrawer): Allow setting of the field to use to track notifications #602

Merged
merged 1 commit into from
Aug 29, 2017

Conversation

jeff-phillips-18
Copy link
Member

Description

This will set the ng-repeat’s ‘track by’ value to the given field in the notification.

No visual changes.

Reported by openshift/origin-web-console#1981

cc @spadgett @benjaminapetersen

… notifications

This will set the ng-repeat’s ‘track by’ value to the given field in
the notification.
@cdcabrera cdcabrera merged commit 3faf39f into patternfly:master Aug 29, 2017
@@ -35,6 +35,7 @@
* @param {boolean} drawerExpanded Flag if the drawer is expanded (only valid if allowExpand is true). Optional, default: false
* @param {string} drawerTitle Title to display for the drawer (leaving this blank will remove the provided expand capability)
* @param {object} notificationGroups Collection notification groups to add to the drawer. Alternatively, a single group object can be given if categorization is not used.
* @param {string} notificationTrackField Optional field from the notifications to use to track by in the notifications listing ($index used otherwise).
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if it matters a ton, but perhaps key instead of field, just for consistency with the lingo Angular uses re: track by. Example:

Duplicates in a repeater are not allowed. Use 'track by' expression to specify unique keys. Repeater: {0}, Duplicate key: {1}, Duplicate value: {2}

From the docs.

@benjaminapetersen
Copy link
Member

I see I missed the merge by 1 minute 😄

@jeff-phillips-18
Copy link
Member Author

Yeah, sorry. If you feel strongly I can update and re-release

@benjaminapetersen
Copy link
Member

I don't think I feel super strong about it, is there anything similar in other components that makes it worth pressing for consistency?

@jeff-phillips-18
Copy link
Member Author

Not that comes to mind.

@benjaminapetersen
Copy link
Member

ok, we prob good to go then! Will be good to pick this up in console +1

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