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

Header component script is not compatible with govuk_template #1270

Closed
alex-ju opened this issue Apr 10, 2019 · 3 comments
Closed

Header component script is not compatible with govuk_template #1270

alex-ju opened this issue Apr 10, 2019 · 3 comments
Assignees
Labels
breaking change 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) 🕔 hours A well understood issue which we expect to take less than a day to resolve.
Milestone

Comments

@alex-ju
Copy link
Contributor

alex-ju commented Apr 10, 2019

Context

govuk_template has an IIFE for header navigation toggle which adds functionality to elements that have a .js-header-toggle class attached to it.

The header component in govuk-frontend uses the same .js-header-toggle class (applied to a button element) to toggle the header.

Problem

Because the govuk_template script is expecting an anchor instead of a button, it tries to get the href attribute and fails to do so if the header component from govuk-frontend is used.

Screen Shot 2019-04-10 at 15 37 29

Potential solution

Prefix or change js- classes to avoid clashed with other scripts (including the deprecated ones).

I'm happy to raise a PR myself since I think I introduced this bit of code, but I need a decision at the team level on what the solution should be.

@kellylee-gds kellylee-gds added awaiting triage Needs triaging by team feature request User requests a new feature and removed feature request User requests a new feature labels Apr 11, 2019
alex-ju added a commit to alphagov/govuk_publishing_components that referenced this issue Apr 15, 2019
This mitigates an issue when we use js-header-toggle for both menu buttons coming from govuk_template (which are marked-up with <a>) and for headers coming from govuk-frontend (marked-up with <button>) (alphagov/govuk-frontend#1270)
alex-ju added a commit to alphagov/govuk_publishing_components that referenced this issue Apr 15, 2019
This mitigates an issue when we use js-header-toggle for both menu buttons coming from govuk_template (which are marked-up with <a>) and for headers coming from govuk-frontend (marked-up with <button>) (alphagov/govuk-frontend#1270)
alex-ju added a commit to alphagov/govuk_publishing_components that referenced this issue Apr 15, 2019
This mitigates an issue when we use js-header-toggle for both menu buttons coming from govuk_template (which are marked-up with <a>) and for headers coming from govuk-frontend (marked-up with <button>) (alphagov/govuk-frontend#1270)
@kellylee-gds kellylee-gds added breaking change 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) and removed awaiting triage Needs triaging by team labels Apr 17, 2019
@NickColley NickColley added this to the v3.0.0 milestone Apr 30, 2019
@NickColley
Copy link
Contributor

Adding this to the 3.0.0 milestone as a point of discussion, potentially we should be reviewing more than just this class and considering if they should be using a 'js-govuk' prefix.

hannalaakso added a commit that referenced this issue May 23, 2019
This will allow us to test compatibility with GOV.UK Frontend
scripts and to diagnose issues such as
#1270

Co-authored-by: Nick Colley <[email protected]>
hannalaakso added a commit that referenced this issue May 24, 2019
This will allow us to test compatibility with GOV.UK Frontend
scripts and to diagnose issues such as
#1270

Co-authored-by: Nick Colley <[email protected]>
@NickColley
Copy link
Contributor

We have decided to change our uses of 'js-' to be 'govuk-js-' as per @hannalaakso 's suggestion.

@kellylee-gds kellylee-gds added 🕔 hours A well understood issue which we expect to take less than a day to resolve. Priority: low labels Jun 5, 2019
@aliuk2012 aliuk2012 self-assigned this Jun 10, 2019
@aliuk2012
Copy link
Contributor

Should be fixed in v3.0.0. Closing this as #1444 has been merged.

aliuk2012 pushed a commit that referenced this issue Jun 14, 2019
This will allow us to test compatibility with GOV.UK Frontend
scripts and to diagnose issues such as
#1270

Co-authored-by: Nick Colley <[email protected]>
aliuk2012 pushed a commit that referenced this issue Jun 14, 2019
This will allow us to test compatibility with GOV.UK Frontend
scripts and to diagnose issues such as
#1270

Co-authored-by: Nick Colley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) 🕔 hours A well understood issue which we expect to take less than a day to resolve.
Projects
Development

No branches or pull requests

4 participants