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

Skipto update and removing jumpto.js #2402

Merged
merged 5 commits into from
Sep 22, 2022
Merged

Skipto update and removing jumpto.js #2402

merged 5 commits into from
Sep 22, 2022

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Jul 5, 2022

This pull request:

  1. Fixes SkipTo.js bug related to Safari rendering of menu items, which was including scrollbars, fixed by using overflow-y: clip.
  2. Removes jumpto.js, since no longer being used
  3. Updated app.js to add scripto.js, instead of jumpto.js.
  4. NOTE: The behavior SkipTo is set to "popup", which is what is being used on the ARIA Practices website. This means the "Skip To Content" button is not visible on page load, you must tab on to the page to see the button.

I would be interested in learning more about how the skipto.js script gets translated and configured (if any) for the APG website.

Jon


WAI Preview Link

@jongund jongund requested review from mcking65 and evmiguel July 5, 2022 16:20
Copy link
Contributor

@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.

Thank you for simplifying the examples and upgrading the older jumpto implementation to the newer skipto implementation. I was able to verify that the issues mentioned in the PR are fixed, and I tested Chrome, Firefox and Safari on Mac and Chrome, Firefox and Edge on Windows. There are some visual differences caused by the configuration being different, and I think I'd vote that we preserve the existing behavior, but I'll leave the decision of how to proceed up to others who might have more context. Thanks!

cspell.json Show resolved Hide resolved
examples/js/skipto.js Show resolved Hide resolved
@alflennik
Copy link
Contributor

The skipto configuration has displayOption set to popup instead of static - just for continuity I think my preference would be to keep it as static.

You asked in the PR description how this file is processed when deployed to the website, and what happens is it does a find and replace on this file to change the displayOption to popup.

@alflennik
Copy link
Contributor

I reran the test several times to make sure the reason the CI is failing is not a flaky test, but it does fail consistently. In the menubar_menubar-navigation there is an error that seems related to this change:

Sending key "ENTER" to menuitem referencing menu with id="id-skip-to-menu-50" in menubar should display submenu

It seems that the HTML ID may have changed due to upgrading the library.

@alflennik alflennik requested review from howard-e and removed request for evmiguel and mcking65 August 31, 2022 15:44
@alflennik
Copy link
Contributor

I just pushed fixes for the issues I identified above. Since I'm contributing to this PR now I assigned @howard-e to help me review. Thanks!

@mcking65
Copy link
Contributor

mcking65 commented Sep 4, 2022

@alflennik

Thank you for the fixes. Is it yet possible to get a netlify preview?

@alflennik
Copy link
Contributor

@mcking65 the preview can be viewed here: w3c/wai-aria-practices#158. I created a PR on the WAI-ARIA Practices Repo because there were some lingering references to jumpto that prevented the preview link from building. It's probably worth noting that the current published version of the APG is already using skipto.

@howard-e
Copy link
Contributor

howard-e commented Sep 8, 2022

Is it yet possible to get a netlify preview?

@mcking65 @alflennik re-ran the action for triggering the WAI Preview Link being generated for this PR given that w3c/wai-aria-practices#158 has been approved and merged. The link is now attached.

Copy link
Contributor

@howard-e howard-e left a comment

Choose a reason for hiding this comment

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

LGTM

@shirsha
Copy link

shirsha commented Sep 22, 2022

There is no change in the appearance of the skip to link in the preview site.

image

@shirsha shirsha self-requested a review September 22, 2022 14:13
Copy link

@shirsha shirsha left a comment

Choose a reason for hiding this comment

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

Skip to link appears the same in production and preview site

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.

6 participants