120 Skiplinks #303

josephgknox · 2019-02-28T23:21:24Z

READY FOR REVIEW

Summary

Adds Skiplinks component and class
Refactors screen reader only class(es), leveraging some Bourbon mixins
General related cleanup

Needed By (Date)

N/A

Urgency

N/A

Steps to Test

Pull this branch
Run grunt styleguide
Review Skiplinks component

Note: the z-index of the brand bar and the Decanter header in the KSS style guide overlay the "Skip to content" button link that slides into view on :focus. Those indexes might need to be temporarily disabled (in inspector) to test this fully.

Affected Projects or Products

Decanter

Associated Issues and/or People

Skip links #120

Homesite uses an ID rather than a class for targeting skiplinks. Do we want to do the same? That is, #su-skiplinks rather than .su-skiplinks?
Do we want to assign a z-index to the Skiplinks component when it slides into view?
If and when this is merged in, do we want to drop our current Skipnav component and related code?

sherakama · 2019-03-01T04:23:41Z

core/templates/components/skiplinks/skiplinks.twig

@@ -0,0 +1,7 @@
+<header>
+  <a href="#main-content" class="su-skiplinks">{{ text|default('Skip to content') }}</a>


Id's are hard and don't really belong in a pattern library. There is no way to ensure that the next developer's software doesn't or does have a main-content id. Unfortunately, for this functionality, we need one. I would suggest opening up the anchor link to a twig variable and perhaps generate a random id if one is not provided.

Example not tested*

{% set skipID = "skip-to-content-" ~ random() % <a href="{{ anchor|default(skipID) }}" class="su-skiplinks">{{ text|default('Skip to content') }}</a>

sherakama · 2019-03-01T04:25:47Z

Do we want to assign a z-index to the Skiplinks component when it slides into view?

I am not sure. I wouldn't want it to be behind anything so, yes? As this is likely above the brand bar perhaps something higher than that component?

If and when this is merged in, do we want to drop our current Skipnav component and related code?

My vote is yes, out with the old, in with the new.

yvonnetangsu · 2019-03-01T18:03:51Z

Homesite uses an ID rather than a class for targeting skiplinks. Do we want to do the same? That is, #su-skiplinks rather than .su-skiplinks?

For the skiplink component itself, I'm totally fine with just using a class .su-skiplink. I believe some sites have multiple skiplinks, eg, one link to content and one to footer, then in that case, it makes even more sense to just use a class.

Do we want to assign a z-index to the Skiplinks component when it slides into view?

Like @sherakama , I do like the skiplinks to be above everything else when it comes into view. I think somewhere in my nav I set something to z-index: 22222 so any number larger than that is probably good 😂

If and when this is merged in, do we want to drop our current Skipnav component and related code?

Yes, I'm totally ok with having just the new skiplink component and drop the old one. It would be nice to be able to pass href variable though.

I do have a question about the twig template. Is it more for demo purposes rather than something that developer will actually pull into their site template? I imagine that the skiplink mixin is easier to use since it doesn't add the header and main sections.

Thanks for working on this, @josephgknox! This is so much better than the old one 👍

josephgknox · 2019-03-01T18:40:48Z

Thank you both for the feedback. I will continue working to implement some of the above, as well as clean up the old Skipnav component.

@yvonnetangsu good question. More so for demo purposes, as I wanted it to be testable in some way via the pattern library. I, too, think that a developer would use the mixin/class/ID/etc. as they wish, leveraging the functionality more than the template.

sherakama · 2019-03-01T18:52:01Z

@josephgknox, for the component itself, have a usable template that could be embedded directly. If you need to show it as an example in the styleguide, create a new template or use straight up markup to change the rendered output example.

yvonnetangsu · 2019-03-01T18:57:57Z

@josephgknox - Great! That makes sense. In that case, would it be helpful to add some wording to explain how one can test the demo in the component section since it's not visible until they start tabbing? Before I started working here, I wasn't familiar with skiplinks and wouldn't know to use the tab key 😂 so perhaps a short explanation would help developers who are new to this.

josephgknox · 2019-03-04T20:04:37Z

@sherakama @yvonnetangsu can you two have another review? We closer?

yvonnetangsu · 2019-03-04T21:43:11Z

@sherakama @yvonnetangsu can you two have another review? We closer?

Yes, definitely closer! A few suggestions - please feel free to push back.

Could we give it a z-index of something like 11222 since I use 11111 for the main nav submenu and it might be better to make sure the skiplink is always above any open submenus?
This is for the styleguide itself
#kssref-components-skiplinks .kss-modifier__example {z-index: 11222;}
might help bring it above the brand bar when it's on focus. I removed the global z-index on the .kss-modifier__example containers a while back since it really messed with the main nav z-index.
Could we add something to the doc along the line of "To see the skiplink component in action, click to the space to the right of the word "Example" in the below container and hit the "tab" key"? (feel free to modify) 😄

core/templates/components/skiplinks/skiplinks.twig

core/templates/components/skiplinks/skiplinks.json

JBCSU · 2019-03-04T21:38:51Z

core/scss/utilities/mixins/accessibility/_skiplinks.scss

+    width: auto;
+    z-index: 9999;
+  }
+}


Why doesn't this extend the helper classes, .su-sr-only-element or .su-sr-only-text, or include the mixins, hide-visually, hide-text?

It can. The approach here, though, is a little bit different than that of what those two mixins (or related classes) do. I used Homesite as an example of an approach and aimed to mirror that implementation, which only "hides" something from print, but does not leverage nor extend the Bourbon mixins (hiding the element or the text).

While @include hide-visually; and @include hide-visually("unhide"); for hover and focus states might get us something similar as far as behavior, it would be a slight shift from what Homesite is doing, I believe.

Related: I noticed that Homesite had a _placeholders.scss file that has a primary purpose of being available for extended code. If we do want to use extends here (or in the future), we might want to consider doing something similar.
https://code.stanford.edu/ucomm/homesite-2017/blob/master/wp-content/themes/homesite17/scss/base/_placeholders.scss

Do you know specifically what would be different if we used hide-visually? Is it the animation of having the skip links drop down from top:-500px?

I'm ok with this as is. I was just curious why we're not using the utilities that were updated as part of this PR.

core/scss/elements/input/_input.scss

JBCSU · 2019-03-04T21:50:44Z

Could we give it a z-index of something like 11222 since I use 11111 for the main nav submenu and it might be better to make sure the skiplink is always above any open submenus?

For identity and homesite, Kevin set up a single separate file that manages all explicit z-indexes for All The Things. I've found it extremely helpful to be able to go to just one place and see the relative "heights" of various elements. I heartily suggest doing that in Decanter. I think it would make it much easier for people building on top of Decanter to find the stack order in a single location so they could know definitely where they need to stick their stuff to show up at the right level.

yvonnetangsu · 2019-03-04T22:39:02Z

Could we give it a z-index of something like 11222 since I use 11111 for the main nav submenu and it might be better to make sure the skiplink is always above any open submenus?

For identity and homesite, Kevin set up a single separate file that manages all explicit z-indexes for All The Things. I've found it extremely helpful to be able to go to just one place and see the relative "heights" of various elements. I heartily suggest doing that in Decanter. I think it would make it much easier for people building on top of Decanter to find the stack order in a single location so they could know definitely where they need to stick their stuff to show up at the right level.

That does sound like a good idea. I wonder what's the best way to do this for Decanter. Maybe have the z-indexes in a variable map and pull the value for each component as needed?

josephgknox · 2019-03-04T23:55:29Z

@yvonnetangsu thanks! I will update the default z-index and will add a partial to the KSS SCSS to target the style guide itself. Also, +1 to some sort of documentation around this. Once things are more "approved" as far as syntax, organization, etc. goes, let's make sure we capture something.

josephgknox · 2019-03-05T00:05:01Z

Could we give it a z-index of something like 11222 since I use 11111 for the main nav submenu and it might be better to make sure the skiplink is always above any open submenus?

For identity and homesite, Kevin set up a single separate file that manages all explicit z-indexes for All The Things. I've found it extremely helpful to be able to go to just one place and see the relative "heights" of various elements. I heartily suggest doing that in Decanter. I think it would make it much easier for people building on top of Decanter to find the stack order in a single location so they could know definitely where they need to stick their stuff to show up at the right level.

I also like this idea! Do we want to break this out into another task? Or, do we want to complete something as part of this task/PR?

yvonnetangsu · 2019-03-05T00:10:05Z

I also like this idea! Do we want to break this out into another task? Or, do we want to complete something as part of this task/PR?

Thanks for the fixes! And I vote for breaking this into another task so we can merge this in faster.

yvonnetangsu

Looks GTG to me! 👍

JBCSU · 2019-03-05T01:16:34Z

Could we give it a z-index of something like 11222 since I use 11111 for the main nav submenu and it might be better to make sure the skiplink is always above any open submenus?

For identity and homesite, Kevin set up a single separate file that manages all explicit z-indexes for All The Things. I've found it extremely helpful to be able to go to just one place and see the relative "heights" of various elements. I heartily suggest doing that in Decanter. I think it would make it much easier for people building on top of Decanter to find the stack order in a single location so they could know definitely where they need to stick their stuff to show up at the right level.

I also like this idea! Do we want to break this out into another task? Or, do we want to complete something as part of this task/PR?

I've created issue #305 to capture this task. Let's not let scope-creep delay merging the good work you've already done.

JBCSU

Thanks for making the changes I requested. And thanks for your great work in general. This looks good to go!

sherakama · 2019-03-05T14:47:48Z

core/templates/components/skiplinks/skiplinks.twig

@@ -0,0 +1,9 @@
+{% set skipID = "skip-to-content-" ~ random() %}
+
+<header>


Do we need a wrapper around this? Isn't the idea that this link be placed as the first element after the body tag? If that is so, I would recommend removing or changing the <header> tag as to not compete with other header elements right after the body tag.

Thanks - this has been removed.

sherakama · 2019-03-05T14:48:16Z

core/templates/components/skiplinks/skiplinks.twig

+{% set skipID = "skip-to-content-" ~ random() %}
+
+<header>
+  <a href="#{{ target_id|default(skipID) }}" class="su-skiplinks">{{ link_text|default('Skip to main content') }}</a>


Missing modifier class.

Missing attributes

sherakama · 2019-03-05T14:49:52Z

core/templates/components/skiplinks/skiplinks.twig

+  <a href="#{{ target_id|default(skipID) }}" class="su-skiplinks">{{ link_text|default('Skip to main content') }}</a>
+</header>
+
+<main>


Do we need to have the target in this template file? It won't be directly usable if this main markup is here. I am assuming this is an example. I would like to see a directly usable option as well as the demo.

@sherakama is your recommendation that the twig template only include the following?

{% set skipID = "skip-to-content-" ~ random() %} <a{{ attributes }} href="#{{ target_id|default(skipID) }}" class="su-skiplinks {{ modifier_class }}">{{ link_text|default('Skip to main content') }}</a>

And, then, perhaps add example markup directly to the SCSS file, something like:

Markup: <a href="#main-content" class="su-skiplinks ">Skip to main content</a><main><h2 id="main-content" class="su-sr-only-text" tabindex="-1">Main Content</h2></main>

I think skiplinks are a special snowflake. They are, by definition, not self-contained. I don't believe there's anyway to get them to work with a single twig file.

Since they'll require the user to create custom markup anyway, I vote for having all the necessary pieces in our .twig file, and document how users need to use each piece. I would therefore vote for restoring the <header> wrapper around the link itself, since it demonstrates the recommended placement for the link.

sherakama

Some feedback around the twig template.

I also like the idea of the z-index file.

* 120-skiplinks-b: Alternative render.

josephgknox · 2019-03-05T19:07:03Z

@yvonnetangsu @JBCSU @sherakama

I think this is now ready for a final review and/or further discussion on any of the recent comments and changes.

sherakama

GTG

yvonnetangsu

Missing a "c" otherwise GTG! Thank you for the great work and adding the documentation 😄

core/scss/components/skiplinks/_skiplinks.scss

Co-Authored-By: josephgknox <[email protected]>

sherakama · 2019-03-05T23:52:47Z

#303 more comments here.

* 120-skiplinks-b: Alternative render.

sherakama · 2019-03-06T01:10:28Z

Three green checkboxes. Merging.

* master: 120 Skiplinks (#303)

* master: Tweaking modular-spacing mixin to be more user friendly (#309) 120 Skiplinks (#303) 72 main nav (#264)

JBCSU · 2019-03-07T18:56:01Z

core/scss/components/skiplinks/_skiplinks.scss

+// </header>
+//  <main>
+//   <h2 id="main-content" class="su-sr-only-text" tabindex="-1">Main Content</h2>
+// </main>


@sherakama Hardcoding the markup here means the styleguide doesn't use our twig templates. In the other branch I refactored the twig files specifically so that the styleguide could use the roll-up skiplinks.twig file, which evaluates to essentially this hardcoded markup, while coding the individual elements separately (link.twig & target.twig) files so site builders could place the elements appropriately in their sites' markup. So is there still a reason to hardcode the markup here? I think having the styleguide use skiplinks.twig, which in turn uses the twig files we're recommending site builders use, is preferable.

I was trying to find a way to balance effort, documentation, and keeping the twig templates directly usable vs an example.

This was a fast (lazy) way to create the correct output in the styleguide

Maintained a usable twig file.

I'd prefer to avoid having twig files merely to generate styleguide output as we can do that in the markup and to keep the intent of the twig files as something that an author can/should/could use directly in their project.

The style guide is how I test the twig files. If they're not actually used in the style guide, how do I know they work? I don't want to have to wait until I build a site on top of decanter to find out there's a problem with the templates.

* master: Add linear gradient background mixin (#304) Tweaking modular-spacing mixin to be more user friendly (#309) 120 Skiplinks (#303) 72 main nav (#264) Update _input.scss (#301) Update README.md (#302) 298-change "sticky-footer" mixin to target <footer> selector (#299) Set base font size for each breakpoint (#295) doc: add period (#294) 151 global footer tweaks (#293) Small bugfix for mobile. (#291) 282: Fix modular-spacing mixins and variables (#290) # Conflicts: # core/js/decanter.js - moved code into core/js/components/main-nav/main-nav.js

Joe Knox added 2 commits February 28, 2019 15:16

adds skiplinks functionality and styles

75e451d

refactor sr only classes

1ce3a10

josephgknox requested review from sherakama, yvonnetangsu and JBCSU February 28, 2019 23:42

sherakama added enhancement Accessibility labels Mar 1, 2019

sherakama added this to the Version 5.0.0 milestone Mar 1, 2019

sherakama reviewed Mar 1, 2019

View reviewed changes

Joe Knox added 2 commits March 1, 2019 10:43

removed old skipnav component and updated skiplinks focus index

d2c2216

skipnav template removal FIXUP

4677e80

Joe Knox added 2 commits March 4, 2019 11:59

update twig template to use variable for better reuse

72ff666

FIXUP

f9f4154

JBCSU reviewed Mar 4, 2019

View reviewed changes

enhance template and json and update z-index

ba16369

yvonnetangsu approved these changes Mar 5, 2019

View reviewed changes

JBCSU approved these changes Mar 5, 2019

View reviewed changes

sherakama reviewed Mar 5, 2019

View reviewed changes

sherakama requested changes Mar 5, 2019

View reviewed changes

Joe Knox and others added 4 commits March 5, 2019 08:59

template file tweak

851fc21

120-skiplinks-b: Alternative render. (#306)

b213cf5

* 120-skiplinks-b: Alternative render.

FIXUP

cb6d067

added documentation for the component to the style guide

200c5ea

sherakama approved these changes Mar 5, 2019

View reviewed changes

yvonnetangsu requested changes Mar 5, 2019

View reviewed changes

core/scss/components/skiplinks/_skiplinks.scss Outdated Show resolved Hide resolved

Update core/scss/components/skiplinks/_skiplinks.scss

f8ff360

Co-Authored-By: josephgknox <[email protected]>

sherakama requested a review from yvonnetangsu March 5, 2019 19:35

yvonnetangsu approved these changes Mar 5, 2019

View reviewed changes

120 skiplinks b (#307)

67221eb

* 120-skiplinks-b: Alternative render.

sherakama merged commit 17c5065 into master Mar 6, 2019

sherakama deleted the 120-skiplinks branch March 6, 2019 01:11

yvonnetangsu added a commit that referenced this pull request Mar 6, 2019

Merge branch 'master' into modular-spacing-tweak-b

3a50ce1

* master: 120 Skiplinks (#303)

yvonnetangsu added a commit that referenced this pull request Mar 6, 2019

Merge branch 'master' into gradient-mixin

38be28a

* master: Tweaking modular-spacing mixin to be more user friendly (#309) 120 Skiplinks (#303) 72 main nav (#264)

JBCSU reviewed Mar 7, 2019

View reviewed changes

		@@ -0,0 +1,7 @@
		<header>
		<a href="#main-content" class="su-skiplinks">{{ text\|default('Skip to content') }}</a>

		@@ -0,0 +1,9 @@
		{% set skipID = "skip-to-content-" ~ random() %}

		<header>

120 Skiplinks #303

120 Skiplinks #303

Conversation

josephgknox commented Feb 28, 2019 • edited Loading

READY FOR REVIEW

Summary

Needed By (Date)

Urgency

Steps to Test

Affected Projects or Products

Associated Issues and/or People

See Also

josephgknox commented Feb 28, 2019

Choose a reason for hiding this comment

sherakama commented Mar 1, 2019 • edited Loading

yvonnetangsu commented Mar 1, 2019

josephgknox commented Mar 1, 2019

sherakama commented Mar 1, 2019

yvonnetangsu commented Mar 1, 2019

josephgknox commented Mar 4, 2019

yvonnetangsu commented Mar 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JBCSU commented Mar 4, 2019 • edited Loading

yvonnetangsu commented Mar 4, 2019 • edited Loading

josephgknox commented Mar 4, 2019

josephgknox commented Mar 5, 2019

yvonnetangsu commented Mar 5, 2019

yvonnetangsu left a comment

Choose a reason for hiding this comment

JBCSU commented Mar 5, 2019

JBCSU left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephgknox Mar 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sherakama left a comment

Choose a reason for hiding this comment

josephgknox commented Mar 5, 2019

sherakama left a comment

Choose a reason for hiding this comment

yvonnetangsu left a comment

Choose a reason for hiding this comment

sherakama commented Mar 5, 2019

sherakama commented Mar 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephgknox commented Feb 28, 2019 •

edited

Loading

sherakama commented Mar 1, 2019 •

edited

Loading

yvonnetangsu commented Mar 4, 2019 •

edited

Loading

JBCSU commented Mar 4, 2019 •

edited

Loading

yvonnetangsu commented Mar 4, 2019 •

edited

Loading

josephgknox Mar 5, 2019 •

edited

Loading