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

feat: added minConnectionType to the options attribute #282

Closed
wants to merge 20 commits into from
Closed

feat: added minConnectionType to the options attribute #282

wants to merge 20 commits into from

Conversation

luccaparadeda
Copy link

@luccaparadeda luccaparadeda commented Feb 16, 2023

a new options that gives developer the power of choosing which type of connection should be the minimum to prefetch or prerender the link, instead of always it being 3g.

Fixes #283

@XhmikosR
Copy link
Collaborator

XhmikosR commented Apr 18, 2023

Please rebase and make sure lint and tests pass after you add tests to cover the new changes.

src/index.mjs Fixed Show fixed Hide fixed
@XhmikosR
Copy link
Collaborator

XhmikosR commented May 3, 2023

I did my best to resolve the conflicts but it needs a proper look from the patch author.

The changes look breaking as they are and new tests are missing.

@Luccatp If you do not update the PR, we'll have to close it, but you can come back later and create a new one.

@XhmikosR
Copy link
Collaborator

XhmikosR commented May 6, 2023

I fixed the conflicts and the breaking change but:

  1. https://github.com/Luccatp/quicklink/blob/feat/minConnectionType/src/chunks.mjs should probably be updated too?
  2. shouldn't 5g be included too?
  3. we need tests

@luccaparadeda
Copy link
Author

I will try to fix in this week time.

The effectiveType uses only this values: https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation/effectiveType#value

Thank you for your collaboration and interest in my PR.

@XhmikosR XhmikosR marked this pull request as draft August 14, 2023 16:10
@luccaparadeda luccaparadeda closed this by deleting the head repository Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

minConnectionType option
2 participants