Skip to content
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

Add support for data-skipto parameters #292

Merged
merged 3 commits into from Feb 6, 2024
Merged

Add support for data-skipto parameters #292

merged 3 commits into from Feb 6, 2024

Conversation

howard-e
Copy link
Contributor

See #259. More specifically #259 (comment) which motivated these changes.

This will require w3c/aria-practices#2807 to be merged first. Also content/patterns/patterns.html and content/patterns/practices.html will have to be updated with the new skipto script as well in aria-practices.

Testing Instructions

Note: I created a testing branch on aria-practices, update-skipto-merged which merges 2807 and aria-practices' main, so there aren't any assumptions that have been missed following w3c/aria-practices#2702 being completed and merged which also plays a part in this work.

  1. Update .gitmodules aria-practices repo branch to update-skipto-merged, like the following:
[submodule "_external/aria-practices"]
  path = _external/aria-practices
  url = https://github.com/w3c/aria-practices.git
  branch = update-skipto-merged
...
  1. Run git submodule --remote --recursive to use the latest sample branch's commit instead.
  2. Follow the README instructions from Install Jekyll onwards to run project as expected.
  3. Observe that the updated skipto script and new parameters are being used.

Copy link

netlify bot commented Jan 10, 2024

Deploy Preview for aria-practices ready!

Name Link
🔨 Latest commit 64d1b52
🔍 Latest deploy log https://app.netlify.com/sites/aria-practices/deploys/65afe9532bc1a100081e6654
😎 Deploy Preview https://deploy-preview-292--aria-practices.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@howard-e howard-e changed the title Update formatForJekyll.js to account for data-skipto parameters Account for data-skipto parameters Jan 10, 2024
@howard-e howard-e changed the title Account for data-skipto parameters Add support for data-skipto parameters Jan 10, 2024
@howard-e howard-e marked this pull request as ready for review January 23, 2024 16:29
Copy link
Collaborator

@alflennik alflennik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clean solution, I like this a lot. I verified that skip-to works on all the different page types, that it's now only loaded once (awesome!), and that the styling and functionality of the different pages aren't impacted.

@howard-e howard-e merged commit c882906 into main Feb 6, 2024
4 checks passed
@howard-e howard-e deleted the update-skipto-load branch February 6, 2024 19:13
@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed pdating skipto.

The full IRC log of that discussion <jugglinmike> Topic: pdating skipto
<jugglinmike> s/ pdating/ Updating/
<jugglinmike> github: https://github.com//pull/292
<jugglinmike> Matt_King: It sounds like there's some work that has to be done for which we do not currently have a pull request
<jugglinmike> Matt_King: Specifically a change in the aria-practices repository
<jugglinmike> howard-e: I suggested to do that in the same pull request which introduces the version updates
<jugglinmike> Matt_King: So you're suggesting that we add a commit to gh-2807?
<jugglinmike> howard-e: That's right--to add the "data-skipto" attributes
<jugglinmike> Matt_King: Can you make that modification?
<jugglinmike> howard-e: Sure
<jugglinmike> howard-e: It already has a couple approvals; would you like me to re-request approval from you and Jem?
<jugglinmike> Matt_King: Yes
<jugglinmike> Matt_King: What's the order of merging, here? Can you merge 292 first, allowing us to preview 2807?
<jugglinmike> howard-e: I think 292 can be merged. If we merge the main branch into 2807, we can subsequently preview that
<jugglinmike> Matt_King: Cool. This will simplify things going forward
<Jem> data-skipto="colorTheme:aria; displayOption:popup; containerElement:div"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants