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: add data-sveltekit-replacestate and -keepfocus options to links #9019

Merged
merged 8 commits into from
Mar 13, 2023

Conversation

axwalker
Copy link
Contributor

@axwalker axwalker commented Feb 13, 2023

An additional data-sveltekit-replacestate option on top of the existing options.

Closes #9014
Closes #7895

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

@@ -1548,7 +1548,7 @@ export function create_client({ target }) {
redirect_chain: [],
details: {
state: {},
replaceState: url.href === location.href
replaceState: options.replace_state || url.href === location.href
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't make the corresponding form changes because I haven't used SvelteKit forms yet - so I don't think I'm best placed to make/verify any changes in that area!

@axwalker axwalker changed the title Add data-sveltekit-replacestate option to links feat: add data-sveltekit-replacestate option to links Feb 13, 2023
@axwalker
Copy link
Contributor Author

Linting is failing because of a TS error in the following:

<a id="one" href="/data-sveltekit/replacestate/target" data-sveltekit-replacestate>one</a>

I couldn't find where I would make the appropriate updates to fix this. I tried looking for corresponding data-sveltekit-noscroll TS definitions but didn't find anything. All I could find are some generated types in test-tmp/.../elements/index.d.ts:

// SvelteKit
'data-sveltekit-noscroll'?: true | '' | 'off' | undefined | null;
'data-sveltekit-preload-code'?: true | '' | 'eager' | 'viewport' | 'hover' | 'tap' | 'off' | undefined | null;
'data-sveltekit-preload-data'?: true | '' | 'hover' | 'tap' | 'off' | undefined | null;
'data-sveltekit-reload'?: true | '' | 'off' | undefined | null;

@axwalker
Copy link
Contributor Author

axwalker commented Feb 13, 2023

Just had another little look at this and realised the corresponding types are within sveltejs/language-tools.

I can make a PR there too but I'm not sure what the process is for ensuring the changes to both would be released in sync?


Update: PR within language-tools here: sveltejs/language-tools#1865

@axwalker
Copy link
Contributor Author

@dummdidumm what's the process for merging in changes across the 3 repos this affects? Will updates to sveltejs/language-tools and sveltejs/svelte get published first and then we can move forward with this PR?

Is there anything else I can do to help on this PR here?

@dummdidumm
Copy link
Member

dummdidumm commented Feb 17, 2023

Yes we will likely merge Svelte/language-tools first.

That said - since you now nicely laid out all the needed PRs across the repos, would you be open to also adding data-sveltekit-keepfocus (would close #7895)? Then we'd have parity with the goto options. (you don't need to add a test for this in my opinion, it's already covered through testing goto)

@axwalker
Copy link
Contributor Author

Sure, I'll try and have a look in the next few days.

@axwalker
Copy link
Contributor Author

I've added -keepfocus, and also made changes for the form handlers. As I said before, I'm not too well versed in Sveltekit forms yet so might be worth checking that part of the PR a bit more closely!

Comment on lines 84 to 87
<a data-sveltekit-keepfocus href="/path">Path</a>
```

...will cause the currently focused element to retain focus after navigation. Otherwise, focus will be reset to the body.
Copy link
Member

Choose a reason for hiding this comment

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

Note to self to try out: I'm wondering if this really keeps focus on the element prior to clicking the a tag, or if the a tag itself is then the focus. Probably the latter, which is why this mainly makes sense for <form> elements, which we probably should note here

Copy link
Member

Choose a reason for hiding this comment

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

I'll give this a closer look later, but I think we should have some sort of note saying this is primarily intended for forms. I think it's usually an antipattern for links, unless you know what you're doing.

Copy link
Member

Choose a reason for hiding this comment

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

Confirmed the currently-focused element is the link itself. I think the language is fine as-is, not sure there's a better way to phrase it that doesn't exclude the form case. I added a suggested paragraph below discussing the a11y considerations.

@geoffrich geoffrich changed the title feat: add data-sveltekit-replacestate option to links feat: add data-sveltekit-replacestate and -keepfocus options to links Feb 22, 2023
Copy link
Member

@dummdidumm dummdidumm 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 your PRs across all three needed repositories to get this in!

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.

Add data-sveltekit-replace link option Create a restore-focus option for progressively-enhanced GET forms
4 participants