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

Extract toolbar management from ApplicationHelper #3333

Merged
merged 1 commit into from
Jul 8, 2015

Conversation

matthewd
Copy link
Contributor

We don't need to define all these globally-visible helper methods -- the usable surface is actually two very narrow methods. Their respective supporting methods don't even overlap.

This is a straightforward relocation -- other than adding some trivial plumbing, the code is all moving untouched.

It's a bit ugly where we have to pass through all the instance variables -- but I suspect that ugliness is a Feature: so much state! 😢

This is only moving things around... but it takes application_helper.rb from 2937 lines to 1250, and application_helper_spec.rb from 4259 to 1447.

self.class.send(:include, ApplicationHelper)
end

def method_missing(sym, *args)
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain what this is for? I'm concerned it will affect other tests (I'm not sure the scoping of this method_missing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's scoped to the containing describe block (which is actually a class, way under the covers).

It allows the below specs to continue working (and looking) just as they did before.

Specifically, it: 1) makes the methods accessible without needing an explicit receiver, and 2) bypasses the fact that they're now all private.

@Fryguy
Copy link
Member

Fryguy commented Jul 1, 2015

@dclarizio or @martinpovolny or @h-kataria Please review.

@martinpovolny
Copy link
Member

There's a trello card https://trello.com/c/1RYJltFP/91-redesign-toolbars-and-toolbar-buttons that I happen to own and there's link to a branch that suggest that I plan a more complex redesign in this area. So while I welcome any help with cleaning up the UI code I would prefer if we do not work one over another on the same thing. There's a bunch of other stuff that noone is working on and we have no cards for it -- such as the expression editor.

Even though this might be a simple relocation I will still have more work figuring out if I did not skip any piece or code as I carry on with the work on the toolbars separation and redesign :-(

@matthewd
Copy link
Contributor Author

matthewd commented Jul 1, 2015

plan a more complex redesign

We seem to have plans for complex redesigns of a great many things. Right now I'm just shooting for "less awful, now", ahead of "much better, later".

Even though this might be a simple relocation I will still have more work figuring out if I did not skip any piece or code as I carry on

I'm not sure I understand this concern. But I'll be happy to take care of rebasing your change for you.

@Fryguy
Copy link
Member

Fryguy commented Jul 1, 2015

@martinpovolny I really like this PR and would love to merge it, even though there might be a conflict with your full blown changes. In particular, it really makes it very clear what the couplings are [1] [2], and removes a TON of methods from the global ApplicationHelper which are very toolbar specific.

Given @matthewd's offer to rebase your branch, are you ok with this being merged?

@martinpovolny
Copy link
Member

@Fryguy : I am no against merging this. I just suggest that next time, it would be good to check the trello for work going on in the particular area that e.g. @matthewd is about to cleanup.

My work is in pretty early stage so thx for the rebase offer, but I will have to handle it somehow.

@dclarizio
Copy link

@matthewd @Fryguy

Either I or @h-kataria will try to give this branch a quick test. As @martinpovolny states, if someone (not just @matthewd) is to embark on some refactoring/cleanup, it might be worthwhile to check the UI Trello board for ongoing or planned work in the area. Can also ping me quickly anytime to see if there is any overlap. Thx, Dan

@miq-bot
Copy link
Member

miq-bot commented Jul 2, 2015

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

We don't need to define all these globally-visible helper methods -- the
usable surface is actually two very narrow methods. Their respective
supporting methods don't even overlap.

This is a straightforward relocation -- other than adding some trivial
plumbing, the code is all moving untouched.
@miq-bot
Copy link
Member

miq-bot commented Jul 6, 2015

Checked commit matthewd@17c98a3 with rubocop 0.32.0
6 files checked, 975 offenses detected

app/helpers/application_helper.rb

app/helpers/application_helper/toolbar_builder.rb

@miq-bot
Copy link
Member

miq-bot commented Jul 6, 2015

...continued

@miq-bot
Copy link
Member

miq-bot commented Jul 6, 2015

...continued

app/helpers/application_helper/toolbar_chooser.rb

spec/helpers/application_helper/toolbar_builder_spec.rb

@miq-bot
Copy link
Member

miq-bot commented Jul 6, 2015

...continued

@miq-bot
Copy link
Member

miq-bot commented Jul 6, 2015

...continued

@miq-bot
Copy link
Member

miq-bot commented Jul 6, 2015

...continued

@chessbyte
Copy link
Member

@martinpovolny @dclarizio Any update on this PR? Agree with @Fryguy that this is a nice refactoring that would be good to get merged, if refactored code works.

@dclarizio
Copy link

Ran the UI in this branch a bit and the toolbars seem to be functioning properly and obeying RBAC.

dclarizio pushed a commit that referenced this pull request Jul 8, 2015
Extract toolbar management from ApplicationHelper
@dclarizio dclarizio merged commit 3646385 into ManageIQ:master Jul 8, 2015
@dclarizio dclarizio added this to the Sprint 26 Ending July 13, 2015 milestone Jul 8, 2015
@matthewd matthewd deleted the toolbar-helpers branch August 22, 2015 18:12
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.

6 participants