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

Transition main navigation bar to new design system. #8381

Merged
merged 4 commits into from
Nov 2, 2023

Conversation

farahTW
Copy link
Contributor

@farahTW farahTW commented Oct 18, 2023

This PR transits main navigation bar to design system.

It does the following:

  • Duplicated existing header partial view to legacy.
  • Created new header partial with design system logic for main navigation.
  • Added new method in base controller to handle toggle between legacy and new header views.
  • Updated admin and design_system views with new header partial view render.
  • Added view tests to BaseController to verify new header renders for design system permissions.

Screen shots:
Development env:
image

Integration env:
image

Production env:
image

Trello:
https://trello.com/c/UFcEeugT/560-transition-main-navigation-bar-to-new-design-system

@farahTW farahTW force-pushed the 560_transition_main_navigation_bar_to_design_system branch from 2ff720a to f8dd84a Compare October 18, 2023 15:38
app/views/layouts/admin.html.erb Outdated Show resolved Hide resolved
app/controllers/admin/base_controller.rb Outdated Show resolved Hide resolved
@farahTW farahTW force-pushed the 560_transition_main_navigation_bar_to_design_system branch from f8dd84a to b28ddfe Compare October 19, 2023 07:09
@@ -9,47 +9,43 @@
navbar-default
navbar-inverse
navbar-static-top
<% if !t('admin.whats_new.show_banner') && admin_template %>add-bottom-margin<% end %>
<% if environment_style %>environment-indicator<% end %>" role="banner">
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove the <header> element at this stage so that we do not nest one of these elements within another, which could affect the way it renders.

I don't think it matters that this will also remove the navigation from header because we are also going to have to adopt a slightly different structure to the existing one when we use the layout_header component, so that the navigation is not structurally part of the header but is a separate <nav> element (but that is part of the next piece of work).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidtrussler
As discussed I have removed the header tag and update the design view. Now the view will look like below and it will get fixed once sub-navigation will get implemented.

image

<%= render partial: "shared/header" %>
<% else %>
<%= render partial: "shared/legacy_header" %>
<% end %>
</div>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will need to be restructured a bit like below so that when using the new header we don't nest it in the classes we don't need (legacy-whitehall, navbar-wrapper etc.)

<% if show_new_header? %>
    <%= render partial: "shared/header" %>
<% else %>
  <div class="legacy-whitehall">
    <div class="environment-<%= environment %> navbar-wrapper">
        <%= render partial: "shared/legacy_header" %>
    </div>
  </div>
<% end %>

@farahTW farahTW force-pushed the 560_transition_main_navigation_bar_to_design_system branch from b28ddfe to 71be6c6 Compare October 19, 2023 11:46
Copy link
Contributor

@davidtrussler davidtrussler left a comment

Choose a reason for hiding this comment

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

The changes made in regard to the comments I left look good to me. 👍

@farahTW farahTW force-pushed the 560_transition_main_navigation_bar_to_design_system branch 2 times, most recently from 92073b5 to 384cf44 Compare October 20, 2023 16:17
@mtaylorgds mtaylorgds force-pushed the 560_transition_main_navigation_bar_to_design_system branch from 384cf44 to 1ee85d7 Compare October 31, 2023 14:56
@farahTW
Copy link
Contributor Author

farahTW commented Nov 1, 2023

Hi @nnagewad !
Please find below screen shots for highlighted tab in main navigation header
Dashboard tab
image

All users tab

image

Users tab
image

farahTW and others added 3 commits November 1, 2023 16:38
This commit includes below changes:
-Duplicated existing header partial view to legacy.
-Created new header partial with design system logic for main navigation.
-Added new method in base controller to handle toggle between legacy and new header views.
-Updated admin and design_system views with new header partial view render.
-Added  view tests to BaseController to verify new header renders for design system permissions.
Updates to use the `NewDocumentController` instead of the
 `DashboardController`, since the `DashboardController` has a dependency
 on a signed-in user.

Moves the login line from setup to each test, since different tests have
 different requirements.
This commit removes govuk grid div styles which seems not required.
@farahTW farahTW force-pushed the 560_transition_main_navigation_bar_to_design_system branch 3 times, most recently from 7799d79 to 37e8e6d Compare November 1, 2023 16:56
@nnagewad
Copy link
Contributor

nnagewad commented Nov 1, 2023

Nice! The changes look great!

This commit includes:
-Update main navigation to show active tabs highlighted.
-Rename the sub-nav helper to header_helper to keep generic.
-Added test to base controller.
@farahTW farahTW force-pushed the 560_transition_main_navigation_bar_to_design_system branch from 37e8e6d to 8d8a9c5 Compare November 2, 2023 11:23
@mtaylorgds
Copy link
Contributor

Latest changes LGTM.

@farahTW farahTW merged commit ee8e3ce into main Nov 2, 2023
15 checks passed
@farahTW farahTW deleted the 560_transition_main_navigation_bar_to_design_system branch November 2, 2023 11:42
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

Successfully merging this pull request may close these issues.

5 participants