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

Add support for layout header component in static #263

Merged
merged 2 commits into from
Feb 15, 2021

Conversation

danacotoran
Copy link
Contributor

During development the markup for the account nav (appears on the transition page, and transition checker) was copied over from the layout header component instead of using the component itself. This was probably due to time constraints.

Screenshot 2021-02-02 at 18 30 08

The component audits are complaining that we are including the assets for layout_header in static, when we are not using the component.

To fix this problem, we are removing the hard coded account navigation from static, and using the layout header component in its place: alphagov/static#2408

That is technically duplicating the header – these changes are here to conditionally swap the #global-header for the layout_header only when the account is enabled and the account nav should be present.

https://trello.com/c/9Kjy1hjb

During development the account nav markup was copied over from the
layout header component instead of using the component itself.
This was probably due to time constraints.

The component audits are complaining that we are including the assets
for layout_header, when we are not using the component.

To fix this problem, we are removing the hard coded account navigation
from static, and using the layout header component in its place. That
would technically be duplicating the header, so these changes are here
to ensure that the layout header is conditionally revealed only when the
account is enabled and the account nav should be present.
@danacotoran danacotoran force-pushed the account-nav-layout-header branch from d537271 to 954fe61 Compare February 12, 2021 13:55
Copy link
Contributor

@barrucadu barrucadu left a comment

Choose a reason for hiding this comment

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

This looks good to me, but are we risking any problems by not having a #global-header on pages with the account navigation?

@danacotoran
Copy link
Contributor Author

Thanks for checking this @barrucadu.
The main thing that comes to mind that could potentially go wrong is tracking, but we've ensured that the account nav links have the correct data attributes so that click tracking should work exactly the same after the switch. I don't think we're at risk of breaking anything else... is there any area in particular that you are concerned about?

Copy link
Contributor

@barrucadu barrucadu left a comment

Choose a reason for hiding this comment

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

No, I don't have any specific areas of concern. If you and Andy can't think of anything that'll go wrong, it's likely fine.

@danacotoran danacotoran merged commit 0ca046b into master Feb 15, 2021
@danacotoran danacotoran deleted the account-nav-layout-header branch February 15, 2021 17:50
@danacotoran danacotoran restored the account-nav-layout-header branch February 15, 2021 17:52
@danacotoran danacotoran deleted the account-nav-layout-header branch February 16, 2021 14:17
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.

2 participants