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

converted initSkipLink to Stimulus and wrote tests for it #9818

Merged
merged 4 commits into from Jan 10, 2023

Conversation

Lovelyfin00
Copy link
Contributor

Fixes #9810

Fix Summary

  • Converted initSkipLink to Stimulus
  • Deleted the initSkipLink files and removed the function from where it's been imported in the code.
  • Wrote test for the SkipController and tested all three use cases. For when the skipLink hasn't been clicked. For when it has been clicked and skips to main content and for when the focus is moved out of the main contents.

After the Skip button has been clicked

image

@squash-labs
Copy link

squash-labs bot commented Dec 22, 2022

Manage this branch in Squash

Test this branch here: https://lovelyfin00skip-link-stimulus-n63t7.squash.io

@lb-
Copy link
Member

lb- commented Dec 22, 2022

Looks like the CI has flagged that your HTML is not formatted - make format-server should fix it.

Copy link
Member

@lb- lb- left a comment

Choose a reason for hiding this comment

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

One additional small formatting issue.

client/src/controllers/SkipLinkController.ts Outdated Show resolved Hide resolved
Copy link
Member

@lb- lb- left a comment

Choose a reason for hiding this comment

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

@Lovelyfin00 this works great!

I have a few changes suggested, which I have made and will push up to your branch, however please read through in detail to understand the changes.

I will check the CI runs and merge in.

I have also added a commit on this branch which just renames the files before adding your changes - I can explain this on our call but please remind me to explain why we do this.

client/src/controllers/SkipLinkController.ts Outdated Show resolved Hide resolved
client/src/controllers/SkipLinkController.ts Outdated Show resolved Hide resolved
client/src/controllers/SkipLinkController.ts Outdated Show resolved Hide resolved
wagtail/admin/templates/wagtailadmin/skeleton.html Outdated Show resolved Hide resolved
Main content
</main>
<button id="other-content">other</button>
`;
Copy link
Member

Choose a reason for hiding this comment

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

Not a huge problem but we do not need to indent this test HTML so much, also the main/main can be in one line.

This just makes it more readable.

lb- and others added 4 commits January 11, 2023 08:17
@lb- lb- merged commit b3a2aec into wagtail:main Jan 10, 2023
lb- pushed a commit to thibaudcolas/wagtail that referenced this pull request Jan 26, 2023
- Use declare for existing Stimulus controllers
- First enabled in wagtail#9761 & then disabled in wagtail#9818
lb- pushed a commit that referenced this pull request Jan 27, 2023
- Use declare for existing Stimulus controllers
- First enabled in #9761 & then disabled in #9818
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

🎛️ Migrate Skip Link to a Stimulus Controller
2 participants