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

Toolbar buttons 3 #6078

Merged
merged 4 commits into from
Jan 18, 2016
Merged

Toolbar buttons 3 #6078

merged 4 commits into from
Jan 18, 2016

Conversation

martinpovolny
Copy link
Member

Extract class for view buttons and move code to history item button.

@martinpovolny martinpovolny changed the title Toolbar buttons 3 [WIP] Toolbar buttons 3 Jan 7, 2016
@martinpovolny martinpovolny mentioned this pull request Jan 11, 2016
Closed
@martinpovolny martinpovolny force-pushed the toolbar_buttons_3 branch 2 times, most recently from 7cf7c32 to 1d57b77 Compare January 13, 2016 09:59
@martinpovolny martinpovolny removed the wip label Jan 15, 2016
@martinpovolny martinpovolny changed the title [WIP] Toolbar buttons 3 Toolbar buttons 3 Jan 15, 2016
@martinpovolny
Copy link
Member Author

@himdel : care to review, please?

@himdel
Copy link
Contributor

himdel commented Jan 15, 2016

@martinpovolny will do.. but unless it's urgent, will do tomorrow, feeling a bit under the weather today..

@martinpovolny
Copy link
Member Author

@himdel : not urgent. Monday is a nice day for the review ;-) Thx!

himdel
himdel reviewed Jan 18, 2016
View reviewed changes
app/helpers/application_helper/toolbar_builder.rb
@@ -116,7 +116,7 @@ def apply_common_props(button, input)
button[:window_url] = "/#{request.parameters["controller"]}#{input[:url]}"
end

dis_title = build_toolbar_disable_button(button['id'])
dis_title = build_toolbar_disable_button(button['child_id'] || button['id'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the fix for the issue in #5884?

EDIT: yes

@himdel
Copy link
Contributor

himdel commented Jan 18, 2016

Apart from the possible issue in build_toolbar_hide_button, LGTM!

@himdel himdel mentioned this pull request Jan 18, 2016
Merged
@martinpovolny
Copy link
Member Author

@himdel : fixes, thx for the review!

@miq-bot
Copy link
Member

miq-bot commented Jan 18, 2016

Checked commits martinpovolny/manageiq@06ae65f~...3915fed with ruby 2.2.3, rubocop 0.34.2, and haml-lint 0.13.0
8 files checked, 4 offenses detected

spec/helpers/application_helper/buttons/history_choice_spec.rb

spec/helpers/application_helper/buttons/history_item_spec.rb

spec/spec_helper.rb

martinpovolny added a commit that referenced this pull request Jan 18, 2016
@martinpovolny
Merge pull request #6078 from martinpovolny/toolbar_buttons_3
051edfd 
Toolbar buttons 3
@martinpovolny martinpovolny merged commit 051edfd into ManageIQ:master Jan 18, 2016
@martinpovolny martinpovolny added this to the Sprint 35 Ending Jan 25, 2016 milestone Jan 18, 2016
@chessbyte
Copy link
Member

@martinpovolny Prefer that author of a PR is not the same as merger of a PR. Next time, please ping @Fryguy, @matthewd or me.

@martinpovolny
Copy link
Member Author

@chessbyte : also should have @himdel explicitly confirm here, that all was checked once again to make it clear that all was checked. He confirmed F2F.

@martinpovolny martinpovolny mentioned this pull request Jan 20, 2016
Closed
@romanblanco romanblanco mentioned this pull request Apr 27, 2016
Merged
@martinpovolny martinpovolny deleted the toolbar_buttons_3 branch November 28, 2017 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
refactoring ui
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@martinpovolny @himdel @miq-bot @chessbyte