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

Set ng-change for tag control options #253

Merged
merged 1 commit into from
Feb 13, 2018

Conversation

syncrou
Copy link
Contributor

@syncrou syncrou commented Feb 12, 2018

Create a function setupCategoryOptions to be called on ng-change

Gigantic Thank you to @eclarizio for all his help!

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

@syncrou
Copy link
Contributor Author

syncrou commented Feb 12, 2018

@miq-bot assign @eclarizio

@miq-bot add_label blocker

@syncrou
Copy link
Contributor Author

syncrou commented Feb 12, 2018

cc - @gmcculloug

@gmcculloug
Copy link
Member

@miq-bot add_labels bug, gaprindashvili/yes

@syncrou
Copy link
Contributor Author

syncrou commented Feb 12, 2018

@miq-bot assign @himdel

@syncrou
Copy link
Contributor Author

syncrou commented Feb 12, 2018

@eclarizio - Please review

@miq-bot miq-bot assigned himdel and unassigned eclarizio Feb 12, 2018
@syncrou syncrou force-pushed the set_tag_control_options_set branch from 63df951 to 1409a55 Compare February 12, 2018 23:14
Create a function setupCategoryOptions to be called on ng-change

https://bugzilla.redhat.com/show_bug.cgi?id=1542590
@syncrou syncrou force-pushed the set_tag_control_options_set branch from 1409a55 to 8d68195 Compare February 13, 2018 00:46
@miq-bot
Copy link
Member

miq-bot commented Feb 13, 2018

Checked commit syncrou@8d68195 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 👍

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.

Just as a bit of background for @himdel: before, all of the category information (id, name, description) was being passed through the API to the core repo when editing a TagControl field, but because the core repo was looking up the category by name and not ID, it wasn't actually changing the category.

Now, @syncrou made a change to the core repo to take all 3 properties into account, and so when selecting from the drop-down, we needed a way to update all 3.

👍

@eclarizio
Copy link
Member

@romanblanco You may want to review this as well, figured I'd tag you since it's in the dialog editor section.

@romanblanco
Copy link
Member

Now, @syncrou made a change to the core repo to take all 3 properties into account

the mentioned change - ManageIQ/manageiq#16965

Copy link
Member

@romanblanco romanblanco left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@JPrause
Copy link
Member

JPrause commented Feb 13, 2018

Is this blocker PR ready to merge?

@romanblanco
Copy link
Member

@himdel @karelhala can you merge?

@dclarizio dclarizio merged commit c07afbc into ManageIQ:master Feb 13, 2018
@dclarizio dclarizio added this to the Sprint 79 Ending Feb 12, 2018 milestone Feb 13, 2018
@himdel
Copy link
Contributor

himdel commented Feb 13, 2018

Backported to gaprindashvili branch:

commit e568c5807684477263682f093162b202599414a0 (HEAD -> gaprindashvili)
Author: Dan Clarizio <[email protected]>
Date:   Tue Feb 13 07:14:14 2018 -0800

    Merge pull request #253 from syncrou/set_tag_control_options_set
    
    Set ng-change for tag control options
    
    (cherry picked from commit c07afbc46a80fb846a2426f1e54726380c7a6290)

himdel pushed a commit that referenced this pull request Feb 13, 2018
Set ng-change for tag control options

(cherry picked from commit c07afbc)
@himdel
Copy link
Contributor

himdel commented Feb 13, 2018

Released in 1.0.13 and 1.1.5.

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.

8 participants