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

Fix layout_header layout and spacing issues #1924

Merged
merged 2 commits into from
Feb 15, 2021

Conversation

danacotoran
Copy link
Contributor

@danacotoran danacotoran commented Feb 11, 2021

What

Fix header navigation layout issue which happens on tablet breakpoints on GOVUK Accounts.
Fix spacing issue which occurs when layout_header is used by applications using static.

Why

Currently, the layout header component looks different in the context of static vs in isolation. This is because this component shares some classes with the hardcoded #global-header from static, as well as some layout classes from the Design System.

  • on the account manager, which does not use static, the layout header navigation does not align as intended with the rest of the header on tablet breakpoints

Before

Screenshot 2021-02-11 at 16 59 55

After

Screenshot 2021-02-11 at 16 57 54

  • in the context of an application that uses static (screenshots below are from finder-frontend), the layout header inherits some styles from static. Additionally, application CSS conflicts with the layout_header CSS resulting in spacing issues, such as extra padding pictured below
Before After
Screenshot 2021-02-11 at 17 09 00 Screenshot 2021-02-11 at 17 09 37

Before

Screenshot 2021-02-11 at 17 01 29

After

Screenshot 2021-02-11 at 17 02 05

This aims to address some of these problems by removing unnecessary classes, as well as increasing layout_header CSS specificity where this CSS might be overruled by external styles from either static or the parent application's stylesheet.

@bevanloon bevanloon temporarily deployed to govuk-publis-adjust-lay-dedfr4 February 11, 2021 17:33 Inactive
@bevanloon bevanloon temporarily deployed to govuk-publis-adjust-lay-dedfr4 February 11, 2021 17:36 Inactive
Copy link
Contributor

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

My main concern with this change is that we are moving away from the Design System component by removing classes (such as govuk-header__content) instead of building on top of it when necessary.

As Accounts doesn't use static, we should be able to use the Design System component (markup and classes) without any issues, and if we need to define additional rules to make the header compatible with static, then we wrap those rules in a compatibility block as you previously did with other elements. By doing so we build on top of that component and we can remove the additional styles at a later stage, rather than diverging.

@danacotoran danacotoran force-pushed the adjust-layout-header-column-padding branch from c986299 to a960388 Compare February 12, 2021 14:03
@bevanloon bevanloon temporarily deployed to govuk-publis-adjust-lay-dedfr4 February 12, 2021 14:03 Inactive
@danacotoran danacotoran force-pushed the adjust-layout-header-column-padding branch from a960388 to 2ab7e9e Compare February 12, 2021 14:59
@bevanloon bevanloon temporarily deployed to govuk-publis-adjust-lay-dedfr4 February 12, 2021 15:00 Inactive
@danacotoran
Copy link
Contributor Author

Thanks @alex-ju, all good points. I think I've managed to make the necessary adjustments without touching the markup.
It's mostly just padding adjustments + a bit of CSS that was missing at the component level (in the context of static these styles would be inherited from static's header-footer-only stylesheet. Accounts doesn't use static so it has some layout issues as a result of those styles not being there).

@danacotoran danacotoran force-pushed the adjust-layout-header-column-padding branch from 2ab7e9e to aefc3e2 Compare February 12, 2021 16:04
@bevanloon bevanloon temporarily deployed to govuk-publis-adjust-lay-dedfr4 February 12, 2021 16:05 Inactive
@alex-ju
Copy link
Contributor

alex-ju commented Feb 12, 2021

Thanks @alex-ju, all good points. I think I've managed to make the necessary adjustments without touching the markup.
It's mostly just padding adjustments + a bit of CSS that was missing at the component level (in the context of static these styles would be inherited from static's header-footer-only stylesheet. Accounts doesn't use static so it has some layout issues as a result of those styles not being there).

Thank you for working on this, @danacotoran! I have tested it on the public and admin layout and fixes mobile/tablet navigation issue as expected. May I suggest we update the changelog entry (and PR description) to be more clear what this fix does (e.g. Fix header navigation layout on mobile/tablet)?

Hopefully, the static overrides will go soon and we'll get the chance to refactor this component and remove such oddities that cause the sole component to be broken.

@injms
Copy link
Contributor

injms commented Feb 15, 2021

I'm seeing a similar problem whilst trying to switch an app to use the gem's public layout (for example, alphagov/calculators#1033 and alphagov/static#2411) - so once the apps aren't using styles from static itself, we should be able to update the component to just work properly. Fingers crossed anyways...

Currently, the layout header component looks different in the context of
static vs in isolation.
- on the account manager, which does not use static, the layout header
navigation does not align as intended with the rest of the header on
tablet devices
- in the context of an application that uses static, the layout header
  inherits some styles from static. Additionally, application CSS will
conflict with the layout_header CSS resulting in spacing issues.

This aims to address some of these problems by removing unnecessary
classes, as well as increasing layout_header CSS specificity where this
CSS might be overruled by external styles from either static or the
parent application's stylesheet.
@danacotoran danacotoran changed the title Fix some layout_header CSS issues Fix layout_header layout and spacing issues Feb 15, 2021
@danacotoran danacotoran changed the title Fix layout_header layout and spacing issues Fix layout_header layout and spacing issues Feb 15, 2021
@danacotoran danacotoran force-pushed the adjust-layout-header-column-padding branch from aefc3e2 to 952e3d1 Compare February 15, 2021 10:57
@bevanloon bevanloon temporarily deployed to govuk-publis-adjust-lay-dedfr4 February 15, 2021 10:57 Inactive
@danacotoran danacotoran merged commit 1787f3b into master Feb 15, 2021
@danacotoran danacotoran deleted the adjust-layout-header-column-padding branch February 15, 2021 11:04
alex-ju added a commit that referenced this pull request Feb 15, 2021
## 24.1.1

* Fix deprecation warnings when running tests ([PR #1899](#1899))
* Update `govuk-frontend` base SCSS imports ([PR #1922](#1922))
* Remove redundant import in accordion component ([PR #1923](#1923))
* Fix toggle click tracking on step-by-steps ([PR #1925](#1925))
* Accordion summary design adjustment ([PR #1926](#1926))
* Fix `layout_header` layout and spacing issues ([PR #1924](#1924))
@alex-ju alex-ju mentioned this pull request Feb 15, 2021
injms added a commit that referenced this pull request Mar 18, 2021
The extra padding was added [1] to avoid a clash in layout when the component
was used in Static. Wrapping the styles in a compatibility mode means that the
layout remains fixed without causing forward compatibility issues.

[1]: #1924
injms added a commit that referenced this pull request Mar 18, 2021
The extra padding was added [1] to avoid a clash in layout when the component
was used in Static. Wrapping the styles in a compatibility mode means that the
layout remains fixed without causing forward compatibility issues.

[1]: #1924
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.

4 participants