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

Infrastructure: Update script that provides skipto menu on all pages to version 5.1 #2619

Merged
merged 4 commits into from
Mar 7, 2023

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Feb 14, 2023

  Changes included in SkipTo version 5.1:
  • Supports including headings and landmarks in custom web components.
  • Rewritten to use Javascript modules.
  • Change configuration options to allow customization of container element to be customized; default container is nav element.
  • The APG implementation of SkipTo sets the container to div so it does not create a navigation landmark.

WAI Preview Link (Last built on Tue, 28 Feb 2023 19:04:02 GMT).

@mcking65
Copy link
Contributor

@jongund

Please edit the opening comment and list changes since V4.2. This will both help with testing and enable us to have a descriptive commit statement in the history.

Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

There are some differences in the attributions in the copyright statements. Please more consistently mirror the language used in the prior version statement. We promised to keep it consistent.

This version:

* Copyright (c) <2022, 2023> (version 5.1) Jon Gunderson, University of Illinois and PayPal
* All rights reserved.

Prior version:

/*! skipto - v4.2.0 - 2022-06-16
 * https://github.com/paypal/skipto
 * Copyright (c) 2022 Jon Gunderson; Licensed BSD
 * Copyright (c) 2021 PayPal Accessibility Team and University of Illinois; Licensed BSD */

@mcking65
Copy link
Contributor

I believe it would be better if the script did not wrap the button in a navigation region. If a page author wants to put the button inside of an existing region, perhaps they can do that. I don't think the script should presume that the author wants to add another region to the page.

Personally, I find regions containing a single element to be pretty annoying. On rare occasion they become necessary because there is an important element in the middle of the page that does not belong in the prior or next region -- that is an extreme edge case and often represents undesirable design.

Because the button is the first element, it is trivial to get to it and almost impossible for screen reader users to overlook. The region is thus adding verbosity that does not provide utility.

If we really wanted it to be inside of a region, which I don't believe is necessary as it is not actual page content, we could make it the first element in the banner. However, I believe it is better that it is outside the banner. If it were persistently visible to all users, I would advocate that it be the first element inside the banner.

@mcking65 mcking65 changed the title update skipto to version 5.1 All Pages: Update script that provides skipto menu to version 5.1 Feb 14, 2023
@mcking65 mcking65 changed the title All Pages: Update script that provides skipto menu to version 5.1 Infrastructure: Update script that provides skipto menu on all pages to version 5.1 Feb 14, 2023
@jongund
Copy link
Contributor Author

jongund commented Feb 17, 2023

@mcking65
I updated the copyright information and removed the navigation landmark wrapping the button and menu.

@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed Infrastructure: Update script that provides skipto menu on all pages to version 5.1 by jongund · Pull Request #2619 · w3c/aria-practices.

The full IRC log of that discussion <jugglinmike> subtopic: Infrastructure: Update script that provides skipto menu on all pages to version 5.1 by jongund · Pull Request #2619 · w3c/aria-practices
<jugglinmike> github: https://github.com//pull/2619
<jugglinmike> Matt_King: I'll review copyright
<jugglinmike> jongund: I talked to someone at Paypal, and they told me that Paypal is ending support for a number of open source projects. The repository referenced in these comments has been abandoned.
<jugglinmike> Jem: I volunteer to perform a functional/visual review?
<jugglinmike> Matt_King: Depending on where this ends up on Friday, it may be possible to include for Monday
<jugglinmike> Matt_King: I don't think we need further legal advice on this one
<Jem> https://github.com//pull/2624

@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed Infrastructure: Update script that provides skipto menu on all pages to version 5.1 by jongund · Pull Request #2619 · w3c/aria-practices.

The full IRC log of that discussion <jugglinmike> Subtopic: Infrastructure: Update script that provides skipto menu on all pages to version 5.1 by jongund · Pull Request #2619 · w3c/aria-practices
<jugglinmike> github: https://github.com//pull/2619
<jugglinmike> Matt_King: I made two changes to the "skipto.js" file. They were strictly editorial--within code comments. Minor typos
<jugglinmike> Jem: I will finish my review by the end of my day today

@mcking65 mcking65 merged commit bc49ef9 into main Mar 7, 2023
@mcking65 mcking65 deleted the update/skipto branch March 7, 2023 18:44
@mcking65 mcking65 added Infrastructure Related to maintaining task force and repo operations, processes, systems, documentation enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy and removed Infrastructure Related to maintaining task force and repo operations, processes, systems, documentation labels Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants