Closed p-galligan closed 3 years ago
Additionally, position: absolute
is layout here, but it seems pretty important to skipLinks in general. Leave it in?
I would have this behave like it should IRL so let's add the visually hidden piece of the puzzle; i.e. this component should only be visible when it's focused.
Regarding positioning - that's a really good question. I'd imagine that the top
and left
values may somewhat depend on other page elements, like if there is a nav. I would say drop the styles in as they are in DIMES and we can review from there.
I also see that in Storybook there are two color options. The orange one you've included and skip-link--blue
. I can't remember the thinking about that to help determine what blue we might use. @helrond , do you remember if there was a specific blue or context for that other color that you had in mind?
I don't recall exactly, but it may have been because I wasn't sure whether the orange would meet the WCAG baseline...or I could just have been going overboard.
I don't recall exactly, but it may have been because I wasn't sure whether the orange would meet the WCAG baseline...or I could just have been going overboard.
Cool. It doesn't meet the WCAG contrast baseline now. I'd vote for us to stick with one color, leave it $flame-orange
, and add the color contrast issue to the "Known Issues" section in the documentation for this component like we have for the button, table of contents, and pagination. Since those all have color contrast fails associated with the $flame-orange
color, it may be easier and more consistent to address them altogether.
I don't recall exactly, but it may have been because I wasn't sure whether the orange would meet the WCAG baseline...or I could just have been going overboard.
Cool. It doesn't meet the WCAG contrast baseline now. I'd vote for us to stick with one color, leave it
$flame-orange
, and add the color contrast issue to the "Known Issues" section in the documentation for this component like we have for the button, table of contents, and pagination. Since those all have color contrast fails associated with the$flame-orange
color, it may be easier and more consistent to address them altogether.
Should I update the documentation to remove the references to orange and blue?
Hmm yeah that's going to take a bit of tweaking, which gets into javascript and such. I can do that, unless you want to wade in a bit more? Basically you just need to return a single SkipLink component rather than an array of them, so you'll want to do something more like the SocialIcons.
OK, so I've done a bunch of things here:
.dimes-skip-link {
@include skip-link($top: 60px, $left: 80px)
}
shown
toggle to add a bunch of inline styles which are the equivalent of show-on-focus
. This is a little messy but I've added some documentation to indicate that those inline style are not to be used in web sites.visually-hidden
and show-on-focus
mixins.Curious what folks think about the mixin and positioning and whether that's useful or overkill.
@p-galligan I can't request your review on this PR since you created it, so this is me requesting your review via a comment.
Yeah, I think this approach works well and it's nice to build in the flexibility to adjust the position with the mixin.
Adds the basic styles for skipLinks.
The visually-hidden portion has not been enabled in storybook.
There was some conversation around this in regards to React. Should we enable it in the storybook handlebars here?
@HaSistrunk @helrond @bonniegee ?