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

Respect swup link selector for preloaded links #122

Merged
merged 9 commits into from
Jan 29, 2024
Merged

Respect swup link selector for preloaded links #122

merged 9 commits into from
Jan 29, 2024

Conversation

daun
Copy link
Member

@daun daun commented Jan 26, 2024

Description

  • Found a branch I've been using as private fork in production for a while
  • Respect and check swup's link selector option
  • The original goal was to support preloading links in SVGs
  • But it's useful in general to only load links that match the link selector

Drive-by changes

  • Return early from swup.preload(url) if no url is passed in (homepage still works as it should be '/' anyway)

Checks

  • The PR is submitted to the master branch
  • The code was linted before pushing (npm run lint)
  • All tests are passing (npm run test)
  • New or updated tests are included
  • The documentation was updated as required

@daun daun requested a review from a team January 26, 2024 16:13
Copy link

github-actions bot commented Jan 26, 2024

Playwright test results

passed  88 passed
skipped  4 skipped

Details

stats  92 tests across 8 suites
duration  1 minute, 33 seconds
commit  54e0f8c

Skipped tests

chromium → preload-plugin.spec.ts → active links → preloads links on touchstart
firefox → preload-plugin.spec.ts → active links → preloads links on touchstart
webkit → preload-plugin.spec.ts → active links → preloads links on touchstart
ios → preload-plugin.spec.ts → active links → preloads links on hover

hirasso
hirasso previously approved these changes Jan 27, 2024
Copy link
Member

@hirasso hirasso left a comment

Choose a reason for hiding this comment

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

Will test this should I ever need it 😄

Do you think this should be mentioned in the docs?

@daun
Copy link
Member Author

daun commented Jan 28, 2024

@hirasso I had to merge from master to fix merge conflicts, please re-approve :)

@daun
Copy link
Member Author

daun commented Jan 28, 2024

@hirasso I'll double-check that this still respects the link selector — this was meant as a fix for setups where swup is configured to load links in SVGs but the preload plugin didn't support that. In that sense, probably no reason to append the docs as we're aiming for the plugin to just work as configured from the core.

hirasso
hirasso previously approved these changes Jan 28, 2024
@daun daun changed the title Support preloading SVG link elements Respect swup link selector for preloaded links Jan 29, 2024
@daun daun requested a review from hirasso January 29, 2024 12:20
@daun
Copy link
Member Author

daun commented Jan 29, 2024

@hirasso Please re-review :) I've added some more tests about respecting swup's link selector and upgraded Playwright — apparently Chrome only started observing single SVG links in IntersectionObserver since the most recent version 121 🤠

@hirasso
Copy link
Member

hirasso commented Jan 29, 2024

Very interesting! That's how we get to be hyper nerds regarding cross-browser knowledge 🤓

@daun daun merged commit d244c33 into master Jan 29, 2024
2 checks passed
@daun daun deleted the svg-links branch January 29, 2024 17:10
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.

2 participants