967: displaying the navbar conditionnaly #968

vurtn · 2021-05-23T14:08:01Z

Sorry for my english.

Why

We need to have the logic about displaying the navbar in one place and not spread all over the controllers (#967).

What

Add a NavbarHelper
Add a method in application controller that call the NavbarHelper
Add the call of the method in controller around the navbar in the layout
Remove the layout without the navbar
Remove the code in the controllers about the old logic
Add tests in navbar_helper_spec.rb

How

I added the function about displaying the navbar in the application_controller because with the inheritance, all controllers will have it. Then i created a NavbarHelper which have the list of the pages concerned about the option and return true or false if the navbar can be display.

Testing

Tests about the navbar helper are in spec/helpers/navbar_helper_spec.rb

Next Steps

I add the current pages (main branch) in the hash and some of them are deleted in others issue (not closed). That's allow us to see clearly what pages had maybe a problem. It should be updated (and close the other issues).

Outstanding Questions, Concerns and Other Notes

Not seem to be concerned.

Accessibility

Not seem to be concerned.

Security

Not seem to be concerned.

Pre-Merge Checklist

I think the reviewer will check this so i let it empty.

Security & accessibility have been considered
Tests have been added, or an explanation has been given why the features cannot be tested
Documentation and comments have been added to the codebase where required
Entry added to CHANGELOG.md if appropriate
Outstanding questions and concerns have been resolved
Any next steps have been turned into Issues or Discussions as appropriate

h-m-m

Hi, @gamecat2d !

Thanks for helping with this. I feel like you really understood some of the suggestions from #966, and I appreciate seeing the tests here. I had a few suggestions for your code. Let me know what you think!

h-m-m · 2021-05-27T23:53:12Z

app/controllers/application_controller.rb

  private

  # TODO: this appears to be unused?
-  def user_not_authenticated(_exception)
+  def user_not_authenticated(exception)


When you have a moment, please undo this change. We begin variable names with _ like this in methods where we have to accept an argument but we do not care about its value

h-m-m · 2021-05-27T23:58:21Z

app/helpers/navbar_helper.rb

+    hidden_places = {}
+    hidden_places['announcements'] = ['new', 'create']


This may be easier to read if you make it a constant. In additional to being easier to read, it should also use RAM more efficiently.

Unrelated, I wonder if we can also think of a better name than HIDDEN_PLACES

HIDEABLE_NAVBAR_ACTIONS?
CAN_HIDE_NAVBAR_FOR?
NAVBAR_HIDEABLE?
Or, since the module is already called NavbarHelper, maybe NavbarHelper::HIDEABLE_ACTIONS? HIDEABLE_ROUTES?

I like using terms that are going to make sense to a lot of different people, so I definitely would like your opinion on this.

Done with NAVBAR_HIDEABLE 👍

h-m-m · 2021-05-28T00:02:31Z

app/controllers/thank_you_controller.rb

@@ -1,8 +1,6 @@
 # frozen_string_literal: true

 class ThankYouController < PublicController
-  layout 'without_navbar'


I always like seeing cleanup like this!

h-m-m · 2021-05-28T00:11:16Z

spec/helpers/navbar_helper_spec.rb

+    end
+
+    it "should return true for the pages not concerned by the displaying option" do
+      expect(subject.show_navbar?('always_display', 'the_navbar', settings))


It might be good here to display data that's more realistic so people can read the test to know what our goals are. For example, setting the first two arguments to fake_test_controller and index might be a better example. Or maybe always_displayed_controller? Whatever you'd like

solebared

This is looking great @gamecat2d , much better than the existing solution.
A couple of thoughts below. Thanks!

solebared · 2021-05-28T00:15:38Z

app/controllers/application_controller.rb

+  def shown?
+    NavbarHelper.show_navbar?(params[:controller], params[:action], context.system_settings.display_navbar?)
+  end
+


I like this direction because it means show_navbar? doesn't need to be mixed into every controller.

Pushing that logic a bit further, i don't think we really need the shown? method either.

We could instead move this logic directly into layouts/application.html.erb since that's the only place it is used:

<%# layouts/application.html.erb %> <% if shown_navbar? controller_name, action_name, context.system_settings.display_navbar? %> <%= render "layouts/navbar" %> <% end %>

This way, nothing extra is added to ApplicationController.

Note also the use of controller_name and action_name above.

solebared · 2021-05-28T00:21:30Z

app/helpers/navbar_helper.rb

@@ -0,0 +1,20 @@
+module NavbarHelper


The way we're using this module, i don't think it belongs in app/helpers, which have a specific usage pattern in rails.
I'd consider moving this to app/lib and naming it NavbarVisibility or some such.

Done and now i understand the difference between Helpers directory and lib ! 👍

solebared · 2021-05-28T00:25:08Z

app/helpers/navbar_helper.rb

@@ -0,0 +1,20 @@
+module NavbarHelper
+  def self.show_navbar?(controller, action, settings)


The param name settings here is ambiguous. From looking at how it's called, i see it comes from context.system_settings.display_navbar?. Perhaps we name it display_navbar to match? Open to other suggestions as well.

solebared

Great adjustments, thank you! I had an idea for one small refactoring but it is completely optional and we can definitely merge as is, once the merge conflicts are resolved.

solebared · 2021-05-28T16:55:58Z

app/lib/navbar_visibility.rb

+  def self.shown_navbar?(controller, action, display_navbar)
+    always_display = NAVBAR_HIDEABLE[controller].nil? || !NAVBAR_HIDEABLE[controller].include?(action)
+    always_display || display_navbar
+  end


One minor refactoring i might suggest:

Suggested change

def self.shown_navbar?(controller, action, display_navbar)

always_display = NAVBAR_HIDEABLE[controller].nil? || !NAVBAR_HIDEABLE[controller].include?(action)

always_display || display_navbar

end

def self.shown?(controller, action, display_navbar)

display_navbar || !hidden?(controller, action)

end

def self.hidden?(controller, action)

NAVBAR_HIDEABLE[controller]&.include?(action)

end

Since we're now using the method with the fully qualified module name, i think we don't need navbar in the method name

Starting the conditional with display_navbar || helps to clarify that the system system takes precedence over controller/action specific checks

IMO, extracting the small hidden? method helps to simplify and make obvious the overall logic.

You are right ! Done.

solebared · 2021-05-28T16:56:26Z

app/views/layouts/application.html.erb

+    <% if NavbarVisibility.shown_navbar?(controller_name, action_name, context.system_settings.display_navbar?) %>
+      <%= render "layouts/navbar" %>
+    <% end %> 


gamecat2d added 2 commits May 23, 2021 15:49

displaying the navbar conditionnaly

de39a94

rubocop

f044d38

h-m-m reviewed May 28, 2021

View reviewed changes

solebared reviewed May 28, 2021

View reviewed changes

feedbacks changes

1aac467

solebared approved these changes May 28, 2021

View reviewed changes

gamecat2d and others added 3 commits May 29, 2021 13:26

feedbacks

dbfb1eb

rubocop

7d7736e

Merge branch 'main' into 967-displaying-navbar

e9d6f9f

h-m-m approved these changes May 29, 2021

View reviewed changes

h-m-m merged commit 6403402 into rubyforgood:main May 29, 2021

h-m-m mentioned this pull request May 29, 2021

770 : Change the display of the navbar in many pages #962

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

967: displaying the navbar conditionnaly #968

967: displaying the navbar conditionnaly #968

vurtn commented May 23, 2021

h-m-m left a comment

h-m-m May 27, 2021

vurtn May 28, 2021

h-m-m May 27, 2021

vurtn May 28, 2021

h-m-m May 28, 2021

h-m-m May 28, 2021

vurtn May 28, 2021

solebared left a comment

solebared May 28, 2021

vurtn May 28, 2021

solebared May 28, 2021

vurtn May 28, 2021

solebared May 28, 2021

vurtn May 28, 2021

solebared left a comment •

edited

Loading

solebared May 28, 2021

vurtn May 29, 2021

solebared May 28, 2021

		hidden_places = {}
		hidden_places['announcements'] = ['new', 'create']

		@@ -0,0 +1,20 @@
		module NavbarHelper
		def self.show_navbar?(controller, action, settings)

-  def self.shown_navbar?(controller, action, display_navbar)
-    always_display = NAVBAR_HIDEABLE[controller].nil? || !NAVBAR_HIDEABLE[controller].include?(action)
-    always_display || display_navbar
-  end
+  def self.shown?(controller, action, display_navbar)
+    display_navbar || !hidden?(controller, action)
+  end
+  def self.hidden?(controller, action)
+    NAVBAR_HIDEABLE[controller]&.include?(action)
+  end

967: displaying the navbar conditionnaly #968

967: displaying the navbar conditionnaly #968

Conversation

vurtn commented May 23, 2021

Why

What

How

Testing

Next Steps

Outstanding Questions, Concerns and Other Notes

Accessibility

Security

Meta

Pre-Merge Checklist

h-m-m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solebared left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solebared left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solebared left a comment •

edited

Loading