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

Allow skiplink target to be overridden #275

Closed
wants to merge 1 commit into from

Conversation

quis
Copy link
Member

@quis quis commented Feb 8, 2017

Fixes #271


GOV.UK Template hardcodes the skiplink to point at #content. But sometimes using #content introduces unwanted styling from 3rd parties (eg in GOV.UK Elements).

This commit allows users of the template to set their own target for the skiplink. They will still need to include the target element in their pages, same as they have to for id='content' at the moment.

@NickColley
Copy link
Contributor

Should we also split this into another issue to avoid styling based on skip links.

I'd propose:

  • Change #content to #govuk-skip-link-target (or something very explicit)
  • Change styling from #content to a specific presentational class.

@quis quis force-pushed the allow-overiding-of-skiplink branch from 02b8069 to d8280bb Compare February 8, 2017 13:31
@quis
Copy link
Member Author

quis commented Feb 8, 2017

@NickColley That would cause breaking changes for consumers of Template and Elements. Feels overkill to fix my rather specific problem – even if the end result would be more robust/self-explanatory.

GOV.UK Template hardcodes the skiplink to point at #content. But
sometimes using `#content` introduces unwanted styling from 3rd parties
(eg [in GOV.UK Elements](https://github.com/alphagov/govuk_elements/blob/d6226bd2c8c7e256f995c0f519daaa4035c6bd54/public/sass/elements/_layout.scss#L9-L20) ).

This commit allows users of the template to set their own target for the
skiplink. They will still need to include the target element in their
pages, same as they have to for `id='content'` at the moment.
@quis quis force-pushed the allow-overiding-of-skiplink branch from d8280bb to 89ae3e1 Compare February 8, 2017 13:36
quis added a commit to alphagov/notifications-admin that referenced this pull request Feb 14, 2017
We didn’t have an element with an `id` of `content` on the product page
because we didn’t want the styles that come with it (from GOV.UK
Elements here: https://github.com/alphagov/govuk_elements/blob/d6226bd2c8c7e256f995c0f519daaa4035c6bd54/public/sass/elements/_layout.scss#L9-L20)

This meant that the skiplink didn’t work on the product page because
it’s target was not on the page. The skiplink’s target is hardcoded to
point at `#content`.

The proper way to fix this would be to not hardcode the skiplink to
point at `#content`, ie what this pull request does:
alphagov/govuk_template#275

Until that is merged, we can hack around the problem by overriding the
styles that Elements applies to `#content`, which is what this pull
request does.
@gemmaleigh
Copy link
Contributor

@quis, would you be able to update this to fix the failing tests?

@timpaul
Copy link
Contributor

timpaul commented Aug 30, 2018

Following the launch of the GOV.UK Design System, GOV.UK Template will now only get major bug fixes and security patches, so I'm going to close this.

The issue that this PR addresses has been captured on GOV.UK Frontend here.

@timpaul timpaul closed this Aug 30, 2018
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.

None yet

4 participants