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

Add the ability to follow links #173

Merged
merged 5 commits into from
Dec 30, 2024

Conversation

nerdachse
Copy link
Contributor

For this we added another Variant to the BlitzEvent Enum and also made
DocumentLike return an Option which currently just has
one Variant for following links.

I consider this a working POC, which needs to be fleshed out.
I didn't invest any time to make sure the DioxusSide still works. It
most likely won't because of the change in the trait definition.

@nicoburns If this is about what you want, I am happy to work on this, as my time allows. If I've diverged too far from your vision, that's also fine!

It's a working POC. When I open the readme app and click a link, the Document is replaced.

 For this we added another Variant to the BlitzEvent Enum and also made
 DocumentLike return an Option<DocumentEvent> which currently just has
 one Variant for following links.

 I consider this a working POC, which needs to be fleshed out.
 I didn't invest any time to make sure the DioxusSide still works. It
 most likely won't because of the change in the trait definition.
@nicoburns
Copy link
Collaborator

@nerdachse I reckon we ought to create a NavigationProvider trait (not entirely set on the name) similar to the NetProvider trait. And then have the Document accept and store an Arc<dyn NavigationProvider>. That trait can have a method on it to navigate to the new page (and we could later extend it with functionality like "refresh page", "exit application", and maybe even "open new window"), and the implementation of that method can use the proxy as this PR already does.

I don't like the way the current implementation uses the return value, because I think it's non-obvious that this is what the return value would, and also because I think it is likely we will want to add more events in future (possibly even with a single handler generating more than one), and I think it would be difficult to allow for multiple events if using the return value.

@nerdachse
Copy link
Contributor Author

I agree, that's why I proposed two different implementation strategies.

I like that your solution has a specific Provider and not just the whole EventLoop exposed to the document.

I'll get to it as soon as I can!

 Instead of bloating the document with another optional Event on
 `DocumentLike`'s' `handle_event` method, we introduce
 a `NavigationProvider` similar to the existing `NetProvider`.

 Currently it only has one method for navigating to a new page, but this
 may change in the future.
@nerdachse
Copy link
Contributor Author

Still not finished for Dioxus side, but I want to iterate on this, to find a satisfying solution.

Happy to hear your feedback @nicoburns!

Copy link
Collaborator

@nicoburns nicoburns left a comment

Choose a reason for hiding this comment

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

The basic structure of this looks good. I have suggested a few tweaks in individual comments. Additionally compiling for some apps (dioxus-native, wpt runner) is broken which will need to be fixed before we can land this. You can check all apps by running cargo build --workspace (or checking the CI output).

packages/blitz-traits/src/navigation.rs Outdated Show resolved Hide resolved
packages/blitz-dom/src/document.rs Outdated Show resolved Hide resolved
packages/blitz-dom/src/document.rs Show resolved Hide resolved
@nerdachse
Copy link
Contributor Author

The basic structure of this looks good. I have suggested a few tweaks in individual comments. Additionally compiling for some apps (dioxus-native, wpt runner) is broken which will need to be fixed before we can land this. You can check all apps by running cargo build --workspace (or checking the CI output).

Sure, I just didn't want to waste time. Now that the path forward is clear, I'll fix it :)

@nerdachse nerdachse requested a review from nicoburns December 27, 2024 19:17
@nicoburns nicoburns changed the title WIP #170 Follow Links - Added the ability to follow links. Add the ability to follow links Dec 27, 2024
@nerdachse
Copy link
Contributor Author

What this explicitly does not do is set the title of the window accordingly. So when you follow a link to another page, the title stays the current one.

@nerdachse
Copy link
Contributor Author

@nicoburns when you're ready to merge, please squash. Thanks :)

@nicoburns nicoburns merged commit 72978dd into DioxusLabs:main Dec 30, 2024
9 checks passed
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