Clean up the "skip-link" component in Bartik

Unassigning so someone can review.

Noticed issue with commenting standards, patch attached

I reviewed the code in patch #3 with CSS coding standards and it looked fine to me.

I agree about removing the whitespace after the comment that zach.bimson posted.

The patch still applies cleanly and the skip-link functionality still works well.

Marking as RTBC

After having a similar patch reviewed and an update suggested, I've created new patches in accordance to the suggestions. The second comments seems unnecessary but ive provided so we can get this though the queue.

Looks good here. The @file docblock does make sense. It states in the manual it should be there, I apologize for missing that on the initial review. https://www.drupal.org/node/1887862#comments

I agree that the second comment is not necessary. The preceding docblock declares that the file contains styles for the skip-link rule sets.

Thanks for the clarity, i guess comments can never be too informative!

Created this... can someone check itIssue

+++ b/core/themes/bartik/css/components/skip-link.css
@@ -1,5 +1,11 @@
+/**
+ * Skip link styles.
+ */

This is redundant when we have the comment above.

I think there is also more we could do to clean up this file. See What to look for when reviewing CSS. I've attached a text file of the whole file so we can review the code using Dreditor

1. .skip-link.visually-hidden.focusable {
   

   Do we need this additional selector? I'm not sure we do

2.   margin-top: 0;
   

   I don't know why we are resetting margin top here because it's not set anywhere

3.   position: absolute !important;
   

   I don't see the need for important here.

4.   width: auto;
   

   Width auto is the default value, I don't know why we have this

5. .skip-link,
   .skip-link:link,
   .skip-link:visited {
   

   Do we need these selectors? It would be to remove them and see if it affects anything.

   Also, is there any reason why we can't merge these properties into the selector above?

6.   line-height: 1.7;
   

   We should add an em unit to the end of here

7.   text-decoration: none;
   

   Text-decoration: none is already set for links in Bartik. I don't htink we need this

8. .skip-link:hover,
   .skip-link:active,
   .skip-link:focus {
     outline: 0;
   }
   

   Can we also try and merge this in to the selector and see if it still works?

I also noticed that we need to unset the border bottom that is being set on all links.

Patch following up on LewisNyman comments.

I've had to keep the .skip-link.visually-hidden.focusable selector in place, as without it, the position is overridden with position:static !important by
visually-hidden.focusable:focus in system.module.css. This is also why position: absolute needs the !important at the end.

That is annoying, but I guess we have to live with it in this issue. I'm happy with the improvements we've made here.

Visual review of the patch - check nothing is broken or the design has been changed.

The hover and focus states now look completely different.

Tested on Chrome and Firefox, seems to be ok, will try looking at some other browsers.
Might have to add the :visited and :hover selectors.

Chrome Screenshots

Firefox Screenshots

The visual change is the border bottom on the skip link component. We did not anticipate finding any css regressions in these issues but as the skip link is mostly hidden, this was a discovery during the work carried out.

The change was suggested in #13 as a regression that has been missed previously.

The border comes from this code..

a:hover, a:active, a:focus, .link:hover, .link:active, .link:focus {
text-decoration: none;
border-bottom-style: solid;
}

The border bottom on the skip link should not exist. Why would we have just a border bottom on a curved element? It does not come from the skip link styles, it is a regression introduced from the generic link styles overpowering this component and no one has picked up on this.

As the skip link does not need a hover state, only appears on focus and it's popping up on the screen is enough visual cue alone as an action, I agree with removing the border. Therefore this issue will contain a visual change for Bartik.

Before:

After:

The skip link still functions as it should. If @LewisNyman is happy with the code cleanup aspect, this issue is RTBC.

But first the patch no longer applies so it needs a reroll!!!

I spent some time trying to remember how to do re rolling from the Sprint at Drupal Camp Brighton.
I think this should be the correct patch.

Thanks! Almost there

+++ b/core/themes/bartik/css/components/skip-link.css
@@ -6,31 +9,23 @@
+}
\ No newline at end of file

We need a blank line at the end of every file, there's a way to set up your text editor config to automatically add a blank line

Ok, newline added.

1. +++ b/core/themes/bartik/css/components/skip-link.css
   @@ -1,4 +1,7 @@
   + * Styles for skip link.
   

   This comment could use a preposition, eg. 'Styles for the skip link.'

2. +++ b/core/themes/bartik/css/components/skip-link.css
   @@ -1,4 +1,7 @@
    .skip-link.visually-hidden.focusable {
   

   This selector is very specific and only overrides the position attribute. Let's move it to a separate selector, so it becomes clear why it is so specific.

3. +++ b/core/themes/bartik/css/components/skip-link.css
   @@ -6,31 +9,23 @@
      margin-top: 0;
   

   This margin can be removed, since the <a> tag has no margin by default.

4. +++ b/core/themes/bartik/css/components/skip-link.css
   @@ -6,31 +9,23 @@
      display: inline-block;
   

   All elements that are position:absolute; are automatically treated as display: block;, so this attribute can be removed.

5. +++ b/core/themes/bartik/css/components/skip-link.css
   @@ -6,31 +9,23 @@
      margin-left: -5.25em; /* LTR */
   

   This attribute is trying to center the element, but only takes an approximation. I suggest using transform: translateX(-50%) to actually center the element.

New Patch following on from idebr's comments.
I had to also move the border-bottom: none and color: #fff into the .skip-link.visually-hidden.focusable

I'm unsure about translateX(-50%) on the margin-left since I don't know how far back browser support is needed, Particularly as Bartick is the default theme. If I can use it i'll make a new patch.

Drupal 8 supports IE9+ and the usual suspects. For reference on transform, see http://caniuse.com/#feat=transforms2d

A few pointers if you are still working on this:

1. +++ b/core/themes/bartik/css/components/skip-link.css
   @@ -1,36 +1,29 @@
      padding: 1px 10px 2px 10px;
   

   The last 10px can be removed, since it is inferred so it becomes padding: 1px 10px 2px;

2. +++ b/core/themes/bartik/css/components/skip-link.css
   @@ -1,36 +1,29 @@
   +  color: #fff;
   +  border-bottom: none;
   

   These styles overrides a:focus. The appropriate selector to override this selector would be .skip-link:focus. Alternatively you can remove the border with a single attribute border-bottom-width: 0; on .skip-link

Thanks idebr, New patch attached.

Thanks for picking this up, andybroomfield! Just a few more things and then this is good to go :)

1. +++ b/core/themes/bartik/css/components/skip-link.css
   @@ -1,36 +1,32 @@
    .skip-link:focus {
   

   This selector needs to stay at .skip-link.visually-hidden.focusable:focus to win specificity over .visually-hidden.focusable:focus

2. +++ b/core/themes/bartik/css/components/skip-link.css
   @@ -1,36 +1,32 @@
   +  border-bottom-width:0;
   

   The declaration has to be separated with a space, eg. border-bottom-width: 0;

3. +++ b/core/themes/bartik/css/components/skip-link.css
   @@ -1,36 +1,32 @@
   +[dir="rtl"] .skip-link {
   +  left: auto;
   +  right: 50%;
   +  -webkit-transform: translateX(-50%);
   +  -ms-transform: translateX(-50%);
   +  transform: translateX(-50%);
    }
   

   The RTL declarations can be removed entirely, since the element is centered anyway. This means the /* LTR */ comments can be removed from the other attributes as well.

Thanks idebr, amended patch attached.

That's quite a cleanup, thanks andybroomfield:). I'll leave this for emma.maria to sign off as she is the maintainer for Bartik.

Screenshots after to prove nothing broke:

I could not get the patch to apply. Needs a reroll.

Reroll after #2398447: Remove the "typography" CSS file in Bartik was committed.

Patch applies ok now.
Adding screenshots to show new patch is working.

Patch applies cleanly.
The skip-link appears on focus and looks as it should, both LTR and RTL as shown by the screenshots in #32.
Visual changes within this issue have been discussed in #18.
The code looks so much cleaner and we have stripped out everything not needed by skip link to function correctly.
I think this issue can be set to RTBC.

Wow, great work all! I tried this out and indeed only saw the skip content link when it received focus.

Committed and pushed to 8.0.x. Thanks!