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

Implement querystring #69

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Implement querystring #69

wants to merge 9 commits into from

Conversation

Heiss
Copy link

@Heiss Heiss commented Mar 2, 2023

Hello,

this are my first steps in yew and wasm. This PR implements my previous request in #67 .

It implements a new router /search/:query and redirect the input to the header. Also it implements a location.replace for input, so it sets the url while you are typing your search query.
Also, I added urlencoding as dependency to handle special characters in url.

Small addition: I fixed the build command, so it works on windows (i am switching between 2 machines), too.

It would be cool to see this in your service.

Have a great day.

Copy link
Owner

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I tried it out and it already works decently.

There's still some things about this that I think should be improved before merging and deploying it though:

  • The history is affected in weird ways by this: When I am on a feature page and type something into the search bar, the browser history entry for the feature page is replaced by the search history entry. I would expect the back button in the browser to take me back to the feature page.
  • When there are multiple consecutive history entries with /search/..., navigating between them with browser back / forwards functions doesn't update the input field value.
  • The search terms will be sent to the server. I do not want the server to see this information, it will turn up in access logs.

I think the way to fix all of these is using a sub-router on the / route, that handles specifically the "hash" of the URL. The URL should then look like http://localhost:8000/#query=search_term. Hashes are not sent to the server, so loading that page again will just request the / route from there, which is good for privacy as well as caching.

I think I saw sub-routers in yew-router before, though I can't find it now. Maybe the Yew Discord will be able to provide some tips, since this is my only project using Yew so I don't have a ton of expertise either.

Cargo.toml Outdated Show resolved Hide resolved
@Heiss
Copy link
Author

Heiss commented Mar 4, 2023

Thank you for your input.

I remove the extra dependency and implement the query via hash. Also i hide the query now on create-event, so it does not involve any big changes to your code except the first lookup.

I place a text about the query feature into the about page, too.

If you want to see, why i wan this feature: I adjust my search app for rust to the new hash implementation already.

@jplatte
Copy link
Owner

jplatte commented Mar 12, 2023

I just tried and it seems to work only for opening /#q=query, not like before where it also updated the URL when searching. That's probably intentional / what you had working locally too?

@Heiss
Copy link
Author

Heiss commented Mar 16, 2023

yes this was on purpose, because something breaks, if i update the query string every keystroke and this is also my first time to interact with yew, so i did not found a fix.
So i remove the querystring from url after the app init, so the UX is the same as before but with a new feature, described in the about page.

@jplatte
Copy link
Owner

jplatte commented Mar 16, 2023

Okay, that makes some sense. However the # still remains after opening the page.. Maybe you can try fixing that? I think then I can merge this, and further features can be added later.

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