BZ1221441 - show unique project name in projects dropdown and project… #4704
Conversation
|
It needs a review from @jwforres @spadgett and @fabianofranz |
|
Thanks for the PR @rafabene. You need to run Also make sure the spec tests are passing with |
| @@ -53,11 +53,16 @@ angular.module('openshiftConsole') | |||
|
|
|||
| sortedProjects = $filter('orderByDisplayName')(projects); | |||
|
|
|||
| options = _.map(sortedProjects, function(item) { | |||
| options = _.map(sortedProjects, function(item, index) { | |||
There was a problem hiding this comment.
That's a lot of calls to the display name filter. Probably faster (and easier to follow) to build a hash of displayNames:count first, then just check against that when building the options. Also, don't show in parens if the display name and project name are the same
There was a problem hiding this comment.
To avoid duplicate code in the directive and in the controller. I'm thinking to build a service for that. What do you think about this proposal ?
There was a problem hiding this comment.
Service seems heavyweight. Filter that takes the item and the list (e.g. uniqueDisplayName(item,items)) would be much cleaner, though it would have to calculate all display names every time
There was a problem hiding this comment.
What about a filter that takes a set of items and returns a map of name:uniqueDisplayName. That lets you calculate once and share code
There was a problem hiding this comment.
Agreed. I'll use the approach of a filter. What about updating the displayName filter ? I think it's preferable over the creation of a new one.
There was a problem hiding this comment.
I'd prefer a second filter that makes use of the displayName filter
|
This is my proposal #2 for review of @jwforres @spadgett and @fabianofranz |
| @@ -94,6 +94,29 @@ angular.module('openshiftConsole') | |||
| return null; | |||
| }; | |||
| }) | |||
| .filter('displayUniqueName', function(displayNameFilter, annotationFilter){ | |||
There was a problem hiding this comment.
drop annotationFilter since you aren't using it
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/5056/) |
|
Thanks for the tip on JS semantics. The code is much more clear now. |
|
[test] |
|
I was not able to understand why there are 2 failing checks. |
|
@rafabene It's saying that |
|
Make sure you clean assets and install assets to pick up dependency updates |
|
[test] |
|
[test] again |
|
Evaluated for origin test up to 39386a9 |
|
[merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/3356/) (Image: devenv-fedora_2374) |
|
Evaluated for origin merge up to 39386a9 |
Merged by openshift-bot
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1221441