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

Refactoring the header #48

Closed
aomran opened this issue Dec 10, 2013 · 3 comments
Closed

Refactoring the header #48

aomran opened this issue Dec 10, 2013 · 3 comments
Assignees

Comments

@aomran
Copy link
Owner

aomran commented Dec 10, 2013

The view partial at https://github.com/aomra015/curri/blob/master/app/views/shared/_header.html.erb is really messy.

  • On line 7 , the <li><%= link_to 'Classrooms', classrooms_path %></li> should be taken out of the if @classroom block. We should also get rid of the if current_page?('/profile/edit') logic on line 33. Lets not complicate the views if we don't have to. The "classrooms" link should in my opinion be consistently displayed in one place on all pages as long as the user is logged in.

  • The unless current_user and if current_user conditionals can be seen as two branches. Either one or the other is displayed. So we should refactor this into two partials like so:

    <% if current_user %>
      <%= render 'logged_in_nav' %>
    <% else %
      <%= render 'logged_out_nav' %>
    <% end %>
    
  • We should also group the teacher-only and student-only navigation components together and again use a two branch conditional approach, creating two partials. This part of the navigation should be in the logged_in_nav partial to keep things consistent.

    <% if current_user.teacher? %>
      <%= render 'teacher_nav' %>
    <% else %
      <%= render 'student_nav' %>
    <% end %>
    

This of course assumes keeping everything in the top menu. I would have done the refactoring myself but I knew that Nachi is working on an alternative way of displaying the menu so it was best to just put my thoughts here.

When designing the menu keep in mind the state of the user. I.e. whether they are logged in or out or whether they are a student or teacher. That way we can build a view that is really easy to work with. Instead of having one header with tons of logic, we break it up into "states":

  • _logged_in_nav.html.erb
  • _logged_out_nav.html.erb
  • teacher_nav.html.erb
  • student_nav.html.erb

The _header.html.erb then serves to in a sense "compile" the navigation by doing a bit of conditional logic. When a front-end developer wants to work with the styles, they can easily do it without worrying about how the navigation logic works.

@ghost ghost assigned nachiketkumar Dec 10, 2013
@nachiketkumar
Copy link
Collaborator

Thanks Ahmed. I am working on an alternate nav layout as you mentioned, so I'll keep all this in mind for the redesign.

@aomran
Copy link
Owner Author

aomran commented Dec 18, 2013

I went ahead and implemented the partials in the latest code. Even if we're going with a different structure for the nav, the new code is easier to understand and work with.

@aomran
Copy link
Owner Author

aomran commented Dec 19, 2013

closing this for now

@aomran aomran closed this as completed Dec 19, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants