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 qnavigatestart and qnavigateend events #2202 #2203

Closed
wants to merge 1 commit into from

Conversation

jwickers
Copy link
Contributor

Signed-off-by: Jeremy Wickersheimer jwickers@gmail.com

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests

Description

Implement the events for issue #2202

Use cases and why

  • makes better UX possible as developpers can now hook up to those events and provide UI feedback like a loader, message if the load is long, etc ...

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Added new tests to cover the fix / functionality

Signed-off-by: Jeremy Wickersheimer <jwickers@gmail.com>
@stackblitz
Copy link

stackblitz bot commented Nov 20, 2022

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@manucorporat
Copy link
Contributor

While i like the idea, this needs a bit of design work before get merged! this would be the first event based API to it sets a precedence, also it global, dispatched in the document, which is not ideal... I agree this is a problem to solve though

@jwickers
Copy link
Contributor Author

Yes, normally I'd also prefer using a store, like the context used by useNavigate for example, and this is what sveltekit does for example https://kit.svelte.dev/docs/modules#$app-stores-navigating

But I couldn't make that work in qwik because I think of the DOM update buffering causing the DOM not to react in real time in that case until the navigation is finished.
If there was a way to expose a store with real-time reactivity this would be an alternative.

Right now using global events seems to be the best way to implement this.

@jwickers
Copy link
Contributor Author

Also I think that using the events we could also make the navigationstart cancellable, so similar to the beforeunload event one could listen to that and on some pages ask user for confirmation before leaving and call prevent default.
In that case qwik could abort the client side navigation.

I don't think this is as easily doable with a store?

@zanettin
Copy link
Contributor

Hi @jwickers 👋
Sorry for the silence on this PR 🙈 i really like the idea and i guess many devs coming from other frameworks are used to these events for showing loaders, etc.

@manucorporat : do you have smth on the radar/roadmap for this or a rough idea of how this could be implemented? also happy to give a hand for this feature ✋ 😄

@zanettin zanettin added the WAITING FOR: user Further information is requested from the issue / pr opener label Feb 16, 2023
@gioboa
Copy link
Member

gioboa commented Oct 10, 2023

@jwickers is this PR closed by #3244? cc @billykwok

@jwickers
Copy link
Contributor Author

I'm not using this PR as we did something else that works well enough using the current APIs, so yeah I'll close it.

@jwickers jwickers closed this Oct 11, 2023
@gioboa
Copy link
Member

gioboa commented Oct 11, 2023

Thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
COMP: qwik-city WAITING FOR: user Further information is requested from the issue / pr opener
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants