WordPress / wp-movies-demo

Demo of the Interactivity API
https://wpmovies.dev
GNU General Public License v2.0
195 stars 42 forks source link

Create a `Movie Search` block #13

Closed michalczaplinski closed 1 year ago

michalczaplinski commented 1 year ago

Adds a Movie Search block that allows users to instantly search for available movies.

It uses the Query Loop block and the server-side rendered markup from the ?post_type=movies&orderby=name&order=asc&s=<search-term> page.

michalczaplinski commented 1 year ago

Here's a summary of the changes on Loom:

SantosGuillamot commented 1 year ago

It looks great, Michal! 👏 I just have a few comments.

Regarding the Movie Search block:

And not only relevant to the Movie Seach block but also to the other ones:

michalczaplinski commented 1 year ago

I believe the focus is not working correctly when you delete your input. I mean, if I start searching and delete everything, I expect to be able to write another search, but the focus is lost.

You're right, good catch. That's because deleting all the search input triggers a navigation back to the home page. And the home page does focus the input on the search box by default.

I wanted the search to behave the way that search on netflix does - when the input is cleared, the user is presented with the main page content again.

I think the solution here is simply to manually focus the input once the navigation back to the home page completes.

Would it make sense to add a buffer to the search, so it only fetches/renders the new page when users stop typing for X milliseconds? At this moment, it's fetching a new page for each letter you type.

Yes and no 😄 . I've tried wrapping the updateURL with a simple debounce function but in the end I've removed it because the user experience is actually a bit worse. I agree though that on a "real" site you would debounce a bit to avoid hitting the backend for every keystroke.

Now that https://github.com/WordPress/block-hydration-experiments/pull/147 has been merged, maybe it makes sense to move the initial state to the server inside the render.php.

Yeah, let's do that 🙂 . Although I haven't updated this repo with the changes from https://github.com/WordPress/block-hydration-experiments/pull/147 yet. That said, I will first try to put the demo inside of an examples folder in https://github.com/WordPress/block-hydration-experiments so as to avoid having to copy paste the latest changes inside of this repo.

SantosGuillamot commented 1 year ago

I think the solution here is simply to manually focus the input once the navigation back to the home page completes.

Do you mean keeping it as it is right now or doing it manually in the code?

I've tried wrapping the updateURL with a simple debounce function but in the end I've removed it because the user experience is actually a bit worse. I agree though that on a "real" site you would debounce a bit to avoid hitting the backend for every keystroke.

Oh, I see! I was suggesting it because it is going to be on a live site at some point and also because some users may copy and paste the code into their sites, once the API is available. Anyway, I don't have a strong opinion on this.

I will first try to put the demo inside of an examples folder in https://github.com/WordPress/block-hydration-experiments so as to avoid having to copy paste the latest changes inside of this repo.

That makes sense 🙂 .

luisherranz commented 1 year ago

I fixed the focus problem in https://github.com/c4rl0sbr4v0/wp-movies-demo/pull/13/commits/8b6bb5626f1fd2abe66c4cc2b50fae6d449f5383. Preact was removing the DOM nodes because there was a mismatch between the tag names, it was <header> in the home (index.html) and <div> in the search (search.html).

Please, when something is not working as expected, don't fix it with a workaround in the code, investigate the root of the problem. We need to make sure that the API is solid.

michalczaplinski commented 1 year ago

Please, when something is not working as expected, don't fix it with a workaround in the code, investigate the root of the problem. We need to make sure that the API is solid

I'm sorry Luis but this sounds a bit condescending - I think we're all aware that we want the API to be as solid as possible and should "try to get to the bottom of the problem". I didn't consider it a workaround - I misunderstood how the API was supposed to work in this case.


All that aside, with the https://github.com/c4rl0sbr4v0/wp-movies-demo/commit/8b6bb5626f1fd2abe66c4cc2b50fae6d449f5383 fix, I think this is good to merge?

luisherranz commented 1 year ago

I'm sorry if that sounded condescending, Michal. Please accept my apologies.

I just want us to be careful with these things because if one of the main selling points of this API is precisely that it can maintain the internal state of the components and the page focus/scroll when navigating from one page to another, we can't show it to the community with an example where we have to imperatively recover the lost focus after a navigation. It kind of defeats the whole purpose.

Again, sorry for my tone.

michalczaplinski commented 1 year ago

@luisherranz Apology accepted. It was only the tone that I objected to.

I understand your concern and I agree that we should figure out the root cause and not imperatively restore the focus. That's also why I mentioned in the video something like "I'm not sure if it's a bug or the intended behavior. I didn't have the time to research it deeply yet". I was confused because I assumed that the headers would be identical.

So yeah, it's all good let's merge this now 🙂