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

Add a type column to the list of images in the pre-prov flow #618

Merged
merged 2 commits into from
Mar 15, 2017
Merged

Add a type column to the list of images in the pre-prov flow #618

merged 2 commits into from
Mar 15, 2017

Conversation

mansam
Copy link
Contributor

@mansam mansam commented Mar 7, 2017

This adds a type column to the pre-provision image-select screen to differentiate images from snapshots.
screenshot from 2017-03-07 17-16-29

Depends upon ManageIQ/manageiq#12970

@chessbyte
Copy link
Member

@gmcculloug @tzumainn please review

@tzumainn
Copy link
Contributor

tzumainn commented Mar 7, 2017

Would it make sense to have the Type column next to the Name column? That matches the other updated Images table. Just let me know if you disagree!

Other than that, looks great! I've verified that it works and properly distinguishes between images and snapshots.

@gmcculloug
Copy link
Member

Would agree with @tzumainn that we should be consistent. Only wish I knew what other "Images table" you were referring to. 😕

@tzumainn
Copy link
Contributor

tzumainn commented Mar 7, 2017

@gmcculloug Ah, you can see it here: ManageIQ/manageiq#12970

It's the listing of images that you get when you click on 'Images' from the providers table.

@dclarizio
Copy link

@tzumainn @gmcculloug any thoughts on moving the column position? Is this ready to go?

@tzumainn
Copy link
Contributor

@mansam can you move the type column to something approximating what you did on the other PR?

@dclarizio this one depends on ManageIQ/manageiq#12970, which has not yet been merged

@mansam
Move "image?" col next to "name" col on the pre-prov view.
3481b46 
This better matches the Cloud Image's column placement.

This commit also makes the pre_prov view entirely responsible
for ordering the columns rather than sharing that responsibility
with the MiqRequestMethods module.
@miq-bot
Copy link
Member

miq-bot commented Mar 13, 2017

Checked commits mansam/manageiq-ui-classic@e83c31f~...3481b46 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 1 offense detected

app/views/miq_request/_pre_prov.html.haml

  • ⚠️ - Line 22 - Missing curly braces around a hash parameter.

@mansam
Copy link
Contributor Author

mansam commented Mar 13, 2017 •
edited
Loading

@tzumainn @dclarizio The type column's been moved to the right of the name column to better match the pending Cloud Image view changes. Changing the column order meant I had to either rearrange the hash in miq_request_methods.rb::build_vm_grid in an ugly way, or I had to refactor the view itself to change the order. Since the view should be doing all the presentation and was already making assumptions about the column ordering, I made the view entirely responsible for that. (It feels better to me to have the ordering all in one place, rather than having to change the header order in build_vm_grid and the actual column order in _pre_prov.html.haml.)

@tzumainn
Copy link
Contributor

👍 re-tested, looks good to me!

@dclarizio dclarizio requested a review from h-kataria March 13, 2017 21:07
h-kataria
h-kataria approved these changes Mar 13, 2017
View reviewed changes
Copy link
Contributor

@h-kataria h-kataria left a comment

Choose a reason for hiding this comment

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

LGTM

@dclarizio
Copy link

@mansam @tzumainn please ping me once the core PR is merged

@tzumainn
Copy link
Contributor

@dclarizio Hi! The PR upon which this depends was merged - can this one be merged now?

@dclarizio dclarizio merged commit 2eb47df into ManageIQ:master Mar 15, 2017
@dclarizio dclarizio added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@h-kataria h-kataria h-kataria approved these changes

Assignees

@dclarizio dclarizio

Labels
compute/cloud enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mansam @chessbyte @tzumainn @gmcculloug @dclarizio @miq-bot @h-kataria