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.

Sign up for GitHub

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

Jump to bottom

Sort bindingsByInstanceRef by first associated application #1742

Merged
merged 2 commits into from
Jun 20, 2017
Merged

Sort bindingsByInstanceRef by first associated application #1742

merged 2 commits into from
Jun 20, 2017

Conversation

benjaminapetersen
Copy link
Contributor

Sort bindingsByInstanceRef by first associated application (from sorted applications)

Per @spadgett suggestion, sorting these in the overview controller so that they do not need to be resorted elsewhere.

screen shot 2017-06-20 at 10 28 53 am

screen shot 2017-06-20 at 10 28 59 am

spadgett
spadgett requested changes Jun 20, 2017
View reviewed changes
app/scripts/controllers/overview.js Outdated
overview.bindingsByInstanceRef = _.reduce(overview.bindingsByInstanceRef, function(result, bindingList, key) {
result[key] = _.sortBy(bindingList, function(binding) {
var apps = _.get(state.applicationsByBinding, [binding.metadata.name]);
var sorted = _.sortBy(apps, 'metadata.name');
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to sort the applicationsByBinding list above instead of here. Then it's also sorted when expanding the provisioned service row in the overview.

@spadgett spadgett mentioned this pull request Jun 20, 2017
Merged
@benjaminapetersen
Copy link
Contributor Author

@spadgett rebased, removed the extra _.sort(). returning the binding name if no apps sorts the secrets. That minor adjustment to objectsByKind is included.

I still am a little uncertain if the kind is handled/should be handled per doing this:
screen shot 2017-06-20 at 10 28 53 am
Or if these just happen to appear neat and tidy due to the naming conventions.

@spadgett
Copy link
Member

[merge][severity:bug]

@openshift-bot
Copy link

Evaluated for origin web console merge up to cea541e

@openshift-bot
Copy link

openshift-bot commented Jun 20, 2017 •
edited
Loading

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/1578/) (Base Commit: 1e79956) (PR Branch Commit: cea541e) (Extended Tests: bug)

@openshift-bot openshift-bot merged commit 5912a3a into openshift:master Jun 20, 2017
@benjaminapetersen benjaminapetersen deleted the sorted-bindings-by-instance-ref branch June 20, 2017 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spadgett spadgett spadgett requested changes

@jwforres jwforres Awaiting requested review from jwforres

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benjaminapetersen @spadgett @openshift-bot