Skip to content

feat: added options to Navigation #11624

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

Closed
wants to merge 4 commits into from
Closed

Conversation

GauBen
Copy link
Contributor

@GauBen GauBen commented Jan 12, 2024

Attempt to implement the feature suggested in #11585

Copy link

changeset-bot bot commented Jan 12, 2024

🦋 Changeset detected

Latest commit: 4d73d91

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@GauBen GauBen changed the title feat: added options to Navigation type feat: added options to Navigation Jan 12, 2024
@GauBen
Copy link
Contributor Author

GauBen commented Jan 12, 2024

Not sure I did everything right but I gave it a shot

@Rich-Harris
Copy link
Member

Thank you! This is an excellent PR and a really nice feature. Only change I might suggest is making Navigation a discriminated union such that options is always defined for goto and link navigations but otherwise undefined:

image

I had a crack at implementing it locally but must have borked it up, so I haven't pushed it. (@dummdidumm may have a view on the best way to implement it.) But what do you think?

@GauBen
Copy link
Contributor Author

GauBen commented Jan 13, 2024

That would make perfect sense, let's try that

@GauBen
Copy link
Contributor Author

GauBen commented Jan 13, 2024

It's not as simple as it seems as some navigations of type link might be external:
https://github.com/GauBen/kit/blob/4d73d916a364b9784c75156a59fec328561386d3/packages/kit/src/runtime/client/client.js#L2017

Also, <form> elements might have data-sveltekit attributes

As with elements, you can set the data-sveltekit-reload, data-sveltekit-replacestate, data-sveltekit-keepfocus and data-sveltekit-noscroll attributes on the

to control the router's behaviour.

We might want to split between InternalNavigation and ExternalNavigation rather than discriminate on type

@eltigerchino eltigerchino marked this pull request as draft July 24, 2024 15:22
@eltigerchino eltigerchino added the feature / enhancement New feature or request label Oct 9, 2024
@GauBen
Copy link
Contributor Author

GauBen commented Feb 8, 2025

I won't go further on this, closing to declutter the PR queue

@GauBen GauBen closed this Feb 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature / enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants