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

Replace control and action checks in page view code with page_is?() #1425

Open
prioux opened this issue Sep 3, 2024 · 6 comments · May be fixed by #1428
Open

Replace control and action checks in page view code with page_is?() #1425

prioux opened this issue Sep 3, 2024 · 6 comments · May be fixed by #1428
Assignees
@MontrealSergiy
Labels
Refactoring User Interface

Comments

@prioux
Copy link
Member

prioux commented Sep 3, 2024 •
edited
Loading

In commit 6030bf1 I added two new
helper methods, page_is?() and page_is_not?(). See their description for more info.

I have used these helpers in one layout page (see them in the same commit), but now we should go through the rest of the
interface code and search for code that say things like

  if params[:controller] == "contname"
  if params[:action] == "actname"
  if params[:controller] == "contname" && params[:action] == "actname"

and replace them by (respectively)

  if page_is?("contname")
  if page_is?("#actname")
  if page_is?("contname#actname")

Since sometimes the pages are checking that a page is NOT a particular controller and action, there is also the method page_is_not?(), which is the equivalent of ! page_is().
@MontrealSergiy
Copy link
Contributor

MontrealSergiy commented Sep 3, 2024 •
edited
Loading

@prioux I can do it even though I find method name page_is? not being ideal (html view template almost always represents a page). More, it is not in ruby style of using a verb with the question mark in a such method name. Even the old way is more readable, although Rail offers dedicated controller_name and action_name helpers.

if controller_name == "contname" && action_name == "actname"

If you like to introduce your own succinct combined helper I would rename action?, not_action? or even controller_and_action?. Other alternatives I can think of are
rendered_from?, controlled_by?, routed_by?, page_on?, page_by?, page_at?

@prioux
Copy link
Member Author

prioux commented Sep 3, 2024 •
edited
Loading

It's just that the code gets really really long when we have something like

   <% if (controller_name == 'userfiles' && action_name == 'index') || (controller_name == 'tasks' && action_name == 'index') %

instead of (I believe) a much more readable

  <% if page_is?("userfiles#index", "tasks#index") %>

If you don't like the names is_page?() for the method, propose another one. I just chose that out of the blue. I want something short though, and with a name that doesn't introduce any ambiguity with anything else (so that's why I don't like controller_and_action (too long) or rendered_from? (confusing with rendering engine))

@prioux prioux changed the title Replace control and action checks in page view code with is_page?() Replace control and action checks in page view code with page_is?() Sep 3, 2024
@prioux
Copy link
Member Author

prioux commented Sep 3, 2024

Ah, I also have errors in my examples here in the issue, the methods are page_is? and page_is_not?

@prioux
Copy link
Member Author

prioux commented Sep 3, 2024

I will correct the description.

@MontrealSergiy
Copy link
Contributor

MontrealSergiy commented Sep 3, 2024 •
edited
Loading

What about control_action? or current_action? (akin to the standard current_page? current_user ...)

still several character longer, yet considering the average template width, I do not think it makes any difference.

Well, by now I am getting used to page_is?, so for now I would go with it, but it you settle on something else, it can be renamed easily, assuming method signature is intact.

MontrealSergiy added a commit to MontrealSergiy/cbrain that referenced this issue Sep 5, 2024
@MontrealSergiy
refactor current controller/action check in templates with new helper a…
4d137cf 
…ces#1425
@MontrealSergiy
Copy link
Contributor

MontrealSergiy commented Sep 6, 2024 •
edited
Loading

While I do no see why

why

  <% if page_is?("userfiles#index", "tasks#index") %>

is more readable than

   <% 
     if ( controller_name == 'userfiles' &&  action_name == 'index' )  || 
        ( controller_name == 'tasks'     &&  action_name == 'index' )

or

   <% 
     if current_page?( controller: 'userfiles',  action: 'index' )  || 
        current_page?( controller: 'tasks',      action: 'index' )

it is certainly way shorter, and faster to type. A PR is submitted.

Once approved I can submit a PR for cbrain-plugins-neuro

@MontrealSergiy MontrealSergiy linked a pull request Sep 6, 2024 that will close this issue
Open
@MontrealSergiy MontrealSergiy linked a pull request Sep 6, 2024 that will close this issue
Use a dedicated helper to check current controller action in views #1425 #1428
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@MontrealSergiy MontrealSergiy

Labels
Refactoring User Interface
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use a dedicated helper to check current controller action in views #1425 MontrealSergiy/cbrain
2 participants
@prioux @MontrealSergiy