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

Automate embedded methods UI #2180

Merged
merged 5 commits into from Oct 13, 2017
Merged

Automate embedded methods UI #2180

merged 5 commits into from Oct 13, 2017

Conversation

pkomanek
Copy link
Contributor

@pkomanek pkomanek commented Sep 15, 2017

Adding the UI for embedded methods in automate

It's based on the following PRs:
ManageIQ/manageiq#14286
ManageIQ/manageiq-automation_engine#16
ManageIQ/manageiq#14847

Method preview from explorer:
method_preview

Embedded methods in method edit:
method_edit

Embedded method select:
method_edit_selecting_embedded_method

@miq-bot miq-bot added the wip label Sep 15, 2017
end

session[:edit] = @edit
end

def pokus
"pokus method"
Copy link
Member

Choose a reason for hiding this comment

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

test code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I forgot to remove it, thank you for comment

if @edit[:action] == 'miq_ae_method_copy'
at_tree_select(:namespace)
else
at_tree_select(:method)
Copy link
Member

Choose a reason for hiding this comment

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

this is repeated multiple times, maybe consider extracting to a common method?

@@ -1,30 +1,51 @@
module AutomateTreeHelper
def at_tree_select_toggle(edit_key)
build_ae_tree(:automate, :automate_tree)
edit_key == :method ? build_ae_tree(:ae_methods, :automate_tree) : build_ae_tree(:automate, :automate_tree)
Copy link
Member

Choose a reason for hiding this comment

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

imho this method is a bit too long, how about breaking it down to multiple smaller methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made it a little smaller, but we are planning to replace this old dialog with a new component and this helper will disappear. So I think that is not necessary to refactor it at this time. (https://www.pivotaltracker.com/n/projects/1613907/stories/151605408)

@gmcculloug
Copy link
Member

@pkomanek Please include a screenshot in description.

cc @mkanoor @tinaafitz

@mkanoor
Copy link
Contributor

mkanoor commented Sep 29, 2017

@pkomanek
The apply button doesn't respond to add the method

screen shot 2017-09-29 at 3 12 14 pm

@pkomanek
Copy link
Contributor Author

pkomanek commented Oct 2, 2017

@mkanoor
I found and fix the bug for the embedded methods dialog in creating a new ae_method. Is it the bug you mentioned?

@miq-bot
Copy link
Member

miq-bot commented Oct 3, 2017

This pull request is not mergeable. Please rebase and repush.

@gmcculloug
Copy link
Member

@pkomanek Please rebase

@miq-bot
Copy link
Member

miq-bot commented Oct 13, 2017

Checked commits pkomanek/manageiq-ui-classic@626b399~...ad57ff2 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
7 files checked, 0 offenses detected
Everything looks fine. 🏆

@pkomanek pkomanek changed the title [WIP]Automate embedded methods UI Automate embedded methods UI Oct 13, 2017
@miq-bot miq-bot removed the wip label Oct 13, 2017
@martinpovolny
Copy link

Works for me.

I found a problematic behavior for copying automate instances and methods while testing this. The UI seems to be stuck for a while w/o the spinner. Actually the spinner stops rotating while is seems something is being done.

@martinpovolny
Copy link

@pkomanek., @mkanoor : the last comment/question was resolved?

@mkanoor
Copy link
Contributor

mkanoor commented Oct 13, 2017

Tested looks good. 👍

@martinpovolny martinpovolny merged commit 3b32359 into ManageIQ:master Oct 13, 2017
@martinpovolny martinpovolny self-assigned this Oct 13, 2017
@martinpovolny martinpovolny added this to the Sprint 71 Ending Oct 16, 2017 milestone Oct 13, 2017
@pkomanek pkomanek deleted the automate_embedded_methods_UI branch November 10, 2017 05:39
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.

None yet

6 participants