-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Fix order and styles for skip link in Twenty Ten #8329
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
@@ -348,6 +348,21 @@ a:hover { | |||
width: 1px; | |||
} | |||
|
|||
a.skip-link:focus { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notes on the ruleset:
- The specificity needs to be higher than
.skip-link:focus
because the stylesheet prints beforeblock-library/style.css
is enqueued. I chosea.skip-link:focus
for a minimal increase, to reduce the chances of overriding custom styles. - This already uses
clip-path
, expecting Trac 62238 to replaceclip
withclip-path
in the.screen-reader-text
ruleset. - The
#333
text color comes from the theme's body text color (in the main content area). - The
16px
font size matches the main content area, which is also in a serif font. If it is any smaller, it probably should be changed to the sans-serif font. - The
line-height
is unitless, and body text has aline-height
of 1.5 times the font size. - The
padding
values are based on thefont-size
andline-height
. - The
background-color
,display
,height
,left
,text-decoration
,top
,width
, andz-index
match the values inblock-library/style.css
.
I experimented with a border to make it stand out more, but that was too much with my browsers' :focus-visible
outline. The skip link could add an outline
like 2px solid #06c
, but that would be inconsistent with any other interactive element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the clip-path: none; intended to override an existing style?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the change to clip-path
was just committed in r59832.
I made this slightly early and needed to sync my branch.
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
Co-authored-by: Mukesh Panchal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
In r59857 |
<div id="wrapper">
.screen-reader-text
andskip-link
classes directly on thea
element.:focus
styles to fit the theme.Trac 14795
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.