Add skip-link to FSE themes #28946
Closed
Add skip-link to FSE themes #28946
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aristath
added
[Feature] Themes
aristath
requested review from
oandregal,
spacedmonkey and
TimothyBJacobs
as code owners
|
Size Change: +553 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
Closed
|
An interesting idea 👍🏻 I have some questions related to the proposal. What if someone wants to have more than 1 skip link? What if they want to have a different text for the link? I'm a bit worried that the simplistic format that
|
Co-authored-by: Timothy Jacobs <[email protected]>
|
Hey, I was looking at this for the code changes, but I believe is better that we clarify direction before diving into code. According to #21507 (comment) it looks like the direction we want to go is trying to absorb the concept of skip links into the template parts (header). I lack context on the specifics but I understand it can be a block or an attribute of the template part ― for which we need the ability to "understand" that a particular template part is a header, which is under development judging by the status of #26599 and a first PR to classify them has recently landed #28410 |
|
The skip link needs to be the first focusable item on a page, no matter where or if a header template part is used. There are also scenarios where a template part is not needed like for a one page site. |
Interesting and valid! I refactored the implementation to allow for multiple skip-links and adjusted the PR description accordingly 👍 |
|
Please include the documentation for the new feature. |
carolinan
approved these changes
Feb 12, 2021
|
Is there anything still blocking this? I guess it needs two approvals? |
|
Added docs. |
|
cc @JustinyAhin for the docs in this PR 👍 |
Docs look good to me. |
|
I believe the docs covers everything. |
|
Though approved, I'd like some more feedback here before merging a change like this. It's important we get it right. 👍 |
gziolo
reviewed
Mar 12, 2021
5 tasks
|
Converted to draft for now, after some discussion #30336 is simpler and more likely to land at this stage. |
|
Closing this one, #30336 was merged instead. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #21507
Description
This PR does the following:
skipLinkstop-level item intheme.jsonThen on render the following happen:
id="skip-link-target"- assuming the user has defined#skip-link-targetas the element in their theme.json file.wp_body_openand target the defined element.theme.jsonThe
skipLinkscan be defined like this:{ "skipLinks": { "auto": true, "css": true, "links": [ { "target": "#skip-link-target", "label": "Skip to Content" }, { "target": "#main-navigation", "label": "Skip to Navigation", "useFallbacks": false } ] } }Arguments:
auto: (bool) If set to false then automatic skip-links generation will be skipped. This is how authors can opt-out of the feature (for example if it's a single-page with no header and it goes straight to content 🤷). If omitted then we assumetrue.css:(bool) We shouldn't force theme-authors to re-invent the wheel each time, so the CSS gets printed by default, but authors can opt-out if they want by setting it tofalse.links: (array) This is where users can define their skip-link (or multiple skip-links if needed). Each "link" can have the following:target: (string|array) An element that should be used as the target.label: (string) The skip-link's label. If omitted defaults toSkip to content.useFallbacks: (bool) If set to false, then the fallbacks won't be added (more info about the fallbacks below). This only makes sense in case we have multiple skip-links defined.Fallback selectors
The fallback selectors are defined in an array:
This covers most (if not all) cases I could think of, and their priority makes sense but we can expand that logic and tweak them.
Why not a separate block?
Adding a separate block for the skip-link and the skip-link target would introduce a lot of complexity for users. The UI would be cumbersome because it's basically invisible elements. Adding as a block would also allow people who don't necessarily understand the purpose or importance of a skip-link to point the skip-link to the wrong things (footer menus anyone?)
This automated implementation takes away all the guesswork, while still allowing users to point the skip-link to an element of their choosing if they want to, by adding the
skip-link-targetanchor to a block. It's a reasonable compromise between automation and freedom.How has this been tested?
Tested in an FSE theme:
Tested both with a
#skip-link-targetelement present, and without.Checklist: