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

Fixed alphabetical order in Labels table #1053

Merged
merged 1 commit into from
May 22, 2017

Conversation

nimrodshn
Copy link
Contributor

Fixed the alphabetical order in the Labels table to suit with openshift.
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1439782

cc: @himdel @zakiva
@simon3z

Before:
screenshot from 2017-04-18 17-10-19
After:
screenshot from 2017-04-18 17-12-39

@nimrodshn nimrodshn force-pushed the fix_alphabetical_order_on_pods branch from 1a9204d to 1ee1590 Compare April 18, 2017 14:37
@himdel
Copy link
Contributor

himdel commented Apr 18, 2017

This looks suspiciously similar to what vm_helper does with labels.

Sounds like we should parametrize textual_labels to not assume custom_attributes , and use that instead.

@himdel
Copy link
Contributor

himdel commented Apr 18, 2017

Perhaps more to the point, @nimrodshn can you do this so that all the label textual summaries use the same approach? (And sort by name.)

@nimrodshn nimrodshn force-pushed the fix_alphabetical_order_on_pods branch from 1ee1590 to 4abd73b Compare April 18, 2017 16:06
@nimrodshn
Copy link
Contributor Author

nimrodshn commented Apr 18, 2017

@himdel fixed - PTAL.
This now make sure every textual summary calling this key-value method will appear in alphabetical order.

@himdel
Copy link
Contributor

himdel commented Apr 19, 2017

Sorry, a misunderstanding .. I didn't mean "make Selector and Node Selector sorted too", I meant: if this "Labels" section is sorted, then all the label sections in the other summaries should be sorted as well, and ideally share the code to do so.

(Not saying Selector and Node Selector should not be sorted, these are container-specific so ..no problem with that.)

@nimrodshn
Copy link
Contributor Author

@himdel looking around there is only one other Labels table (at app/helpers/vm_helper/textual_summary.rb) which is already sorted.

@himdel
Copy link
Contributor

himdel commented May 9, 2017

@nimrodshn There's also Docker Labels in app/helpers/container_image_helper/textual_summary.rb.. but my point was not "make them all sorted", it was "make all of them sorted the same way, using the same methods".

Right now, the old one is sorted because textual_labels calls @record.custom_attributes, sorts them by name, ale converts to a hash, while the other one is sorted because textual_key_value_group is called on @record.labels and the result is sorted by label.

Either is fine, both are not :).

(And I get that the labels vs custom_attributes difference is probably not something you can address, but I'm sure you can make all of them use the same code to do the sort + convert to hash bit.)

@nimrodshn
Copy link
Contributor Author

@himdel now it's clear to me! sorry for the missunderstanding...

@nimrodshn nimrodshn force-pushed the fix_alphabetical_order_on_pods branch from b8589f8 to ddbe861 Compare May 15, 2017 09:00
@nimrodshn
Copy link
Contributor Author

@himdel Now they are all using textual_key_value_group for consistency. PTAL.

@nimrodshn nimrodshn force-pushed the fix_alphabetical_order_on_pods branch from ddbe861 to d8832c8 Compare May 15, 2017 12:04
@himdel
Copy link
Contributor

himdel commented May 18, 2017

@nimrodshn Looks great, thanks!

Just 2 more things...

  • the old textual_labels is now dead code and can be removed
  • can you move that method to TextualSummaryHelper please? So that we don't have to include a Container-specific helper in a Vm-specific helper.

@nimrodshn nimrodshn force-pushed the fix_alphabetical_order_on_pods branch from d8832c8 to 84a561d Compare May 22, 2017 08:34
@nimrodshn
Copy link
Contributor Author

@himdel PTAL.

@nimrodshn nimrodshn force-pushed the fix_alphabetical_order_on_pods branch from 84a561d to f3f4c15 Compare May 22, 2017 08:40
@@ -1,6 +1,6 @@
module ContainerSummaryHelper
include TextualMixins::TextualName

include TextualSummaryHelper
Copy link
Contributor

Choose a reason for hiding this comment

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

You can safely remove this (and the one in vm_helper), TextualSummaryHelper is included from ApplicationHelper and thus always available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@himdel Done. 👍

@nimrodshn nimrodshn force-pushed the fix_alphabetical_order_on_pods branch from f3f4c15 to 00a9686 Compare May 22, 2017 09:06
refactored

Refactoring

make labels sorted

changed methods for consistency

changed method for vm_helper labels

cleaning up dead code and moving some methods

refactoring

refactoring

refactoring
@nimrodshn nimrodshn force-pushed the fix_alphabetical_order_on_pods branch from 00a9686 to 30ffc34 Compare May 22, 2017 09:11
@nimrodshn
Copy link
Contributor Author

@himdel PTAL [ again :) ]

@miq-bot
Copy link
Member

miq-bot commented May 22, 2017

Checked commit nimrodshn@30ffc34 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 👍

@himdel himdel merged commit bc1d201 into ManageIQ:master May 22, 2017
@himdel himdel added this to the Sprint 61 Ending May 22, 2017 milestone May 22, 2017
@himdel
Copy link
Contributor

himdel commented May 22, 2017

Thanks @nimrodshn , merged :)

@nimrodshn
Copy link
Contributor Author

@himdel hooray!! :)

simaishi pushed a commit that referenced this pull request Jun 8, 2017
@simaishi
Copy link
Contributor

simaishi commented Jun 8, 2017

Fine backport details:

$ git log -1
commit b9f321b28a650e080debe5c7a1555ba2189e63b5
Author: Martin Hradil <[email protected]>
Date:   Mon May 22 11:43:00 2017 +0000

    Merge pull request #1053 from nimrodshn/fix_alphabetical_order_on_pods
    
    Fixed alphabetical order in Labels table
    (cherry picked from commit bc1d201ab23f20628f3e7b97855363fa9bb205e0)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1460023

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

Successfully merging this pull request may close these issues.

5 participants