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

Show additional fields for dynamic fields #1199

Merged
merged 1 commit into from
May 2, 2017
Merged

Show additional fields for dynamic fields #1199

merged 1 commit into from
May 2, 2017

Conversation

h-kataria
Copy link
Contributor

@h-kataria h-kataria commented Apr 28, 2017 •
edited
Loading

Made changes to show additional fields for dynamic fields. Additional field sunder Options box are displayed as well as new box with label "Overridable Options" appears for dynamic fields. More details on these fields are in BZ comment#28(https://bugzilla.redhat.com/show_bug.cgi?id=1431692#c28)

https://bugzilla.redhat.com/show_bug.cgi?id=1431692

before:
dynamic_textbox_before
dynamic_dropdown_before

after:
dynamic_textbox_after
dynamic_dropdown_after

@gmcculloug please review/test

@h-kataria h-kataria changed the title Show additional fields for dynamic textbox/dropdown fields [WIP] - Show additional fields for dynamic textbox/dropdown fields May 1, 2017
@miq-bot miq-bot added the wip label May 1, 2017
@h-kataria h-kataria force-pushed the dynamic_dialog_field_options_fix branch from d97ff33 to e9c25fc Compare May 1, 2017 16:40
@h-kataria h-kataria changed the title [WIP] - Show additional fields for dynamic textbox/dropdown fields Show additional fields for dynamic fields May 1, 2017
@miq-bot miq-bot removed the wip label May 1, 2017
@h-kataria h-kataria force-pushed the dynamic_dialog_field_options_fix branch 2 times, most recently from 299e990 to c3f43b3 Compare May 1, 2017 20:37
@h-kataria
Show additional fields for dynamic fields
eafd24e 
Made changes to show additional fields for dynamic fields. Additional  field sunder Options box are displayed as well as new box with label "Overridable Options" appears for dynamic fields. More details on these fields are in BZ comment#28(https://bugzilla.redhat.com/show_bug.cgi?id=1431692#c28)

https://bugzilla.redhat.com/show_bug.cgi?id=1431692
@h-kataria h-kataria force-pushed the dynamic_dialog_field_options_fix branch from c3f43b3 to eafd24e Compare May 1, 2017 21:11
@miq-bot
Copy link
Member

miq-bot commented May 1, 2017

Checked commit h-kataria@eafd24e with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

@gmcculloug
Copy link
Member

@eclarizio After a few rounds I think we got the fields showing correctly now. Please review.

eclarizio
eclarizio approved these changes May 2, 2017
View reviewed changes
Copy link
Member

@eclarizio eclarizio left a comment

Choose a reason for hiding this comment

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

Looks good to me, one small comment about a potential refactoring that can be done in a future PR.

👍

app/views/miq_ae_customization/_dialog_field_form_dynamic_options.html.haml
@@ -28,4 +28,91 @@
= check_box_tag('field_auto_refresh', '1',
@edit[:field_auto_refresh],
'data-miq_observe_checkbox' => {:url => url}.to_json)
.form-group
Copy link
Member

Choose a reason for hiding this comment

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

This one stands out to me, but a lot of these .form-groups are duplicated now, right? Should we maybe be looking at separating them into their own partials in the future so that we don't accidentally forget to change both places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eclarizio i agree, and I have that on my mind to refactor this, so in future we don't have to maintain these fields in 2 places.

@dclarizio dclarizio merged commit ed444a8 into ManageIQ:master May 2, 2017
@dclarizio dclarizio added this to the Sprint 60 Ending May 8, 2017 milestone May 2, 2017
simaishi pushed a commit that referenced this pull request May 2, 2017
@simaishi
Merge pull request #1199 from h-kataria/dynamic_dialog_field_options_fix
98321a2 
Show additional fields for dynamic fields
(cherry picked from commit ed444a8)

https://bugzilla.redhat.com/show_bug.cgi?id=1447391
@simaishi
Copy link
Contributor

simaishi commented May 2, 2017

Fine backport details:

$ git log -1
commit 98321a2841d6bdc93101b3e747ef4c8239401463
Author: Dan Clarizio <dclarizi@redhat.com>
Date:   Tue May 2 08:12:11 2017 -0700

    Merge pull request #1199 from h-kataria/dynamic_dialog_field_options_fix
    
    Show additional fields for dynamic fields
    (cherry picked from commit ed444a80aa038e0fd2016a6668cb39d73bd7974b)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1447391

@h-kataria h-kataria deleted the dynamic_dialog_field_options_fix branch May 3, 2017 15:46
@simaishi
Copy link
Contributor

simaishi commented May 9, 2017

@h-kataria @dclarizio can this be euwe/yes (as per flag on the BZ)?

@dclarizio
Copy link

@simaishi I'll leave that up to @h-kataria as there's quite a bit of code here.

@gmcculloug
Copy link
Member

I'm good with the back-port if @h-kataria is.

@h-kataria
Copy link
Contributor Author

@simaishi i am good with back-porting.

@simaishi
Copy link
Contributor

Euwe bakcport (to manageiq repo) details:

$ git log -1
commit faf220e85c2e5d1bcca24f146a0436a2ab9d4a23
Author: Dan Clarizio <dclarizi@redhat.com>
Date:   Tue May 2 08:12:11 2017 -0700

    Merge pull request #1199 from h-kataria/dynamic_dialog_field_options_fix
    
    Show additional fields for dynamic fields
    (cherry picked from commit ed444a80aa038e0fd2016a6668cb39d73bd7974b)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1450421

@romanblanco romanblanco mentioned this pull request Sep 27, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eclarizio eclarizio eclarizio approved these changes

Assignees

@dclarizio dclarizio

Labels
blocker enhancement euwe/backported fine/backported services
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@h-kataria @miq-bot @gmcculloug @simaishi @dclarizio @eclarizio