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

120-skiplinks-b: Alternative render. #306

Merged
merged 4 commits into from
Mar 5, 2019
Merged

Conversation

sherakama
Copy link
Member

@JBCSU @yvonnetangsu @josephgknox

How about something like this. With this approach, the twig template is usable directly in your work and the rendered output has the example we want.

Thoughts?

@yvonnetangsu
Copy link
Member

@JBCSU @yvonnetangsu @josephgknox

How about something like this. With this approach, the twig template is usable directly in your work and the rendered output has the example we want.

Thoughts?

I do like this approach! Btw, I just noticed this now, should we make this singular "skiplink" so it's consistent with all the other components?

Copy link
Member

@yvonnetangsu yvonnetangsu left a comment

Choose a reason for hiding this comment

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

I think the id is actually something like "main-content" instead of "skip-to-x". The json file was updated so this markup in the styleguide should probably reflect that too.

core/scss/components/skiplinks/_skiplinks.scss Outdated Show resolved Hide resolved
core/scss/components/skiplinks/_skiplinks.scss Outdated Show resolved Hide resolved
@josephgknox
Copy link

@sherakama this approach also works well for me. I'll merge this into the other branch. We can continue discussion there if needed.

@yvonnetangsu it's a good question regarding the component naming convention. I'd argue that this accessibility-related component might be OK to leave as plural, as the chances of this not skipping more than one link (used primarily for skipping navigation) is rare. Also, it appears to most commonly be referred to as Skiplinks on the inter-webs. Willing to discuss this more, too. I might be crazy. :)

@josephgknox josephgknox merged commit b213cf5 into 120-skiplinks Mar 5, 2019
@josephgknox josephgknox deleted the 120-skiplinks-b branch March 5, 2019 18:49
@yvonnetangsu
Copy link
Member

@josephgknox I'm comfortable going with the most commonly known term 😸 Thanks for checking!

@JBCSU
Copy link
Collaborator

JBCSU commented Mar 5, 2019

@sherakama @josephgknox I just committed to this branch an alternative that I think works quite well. Please let me know what you think.

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