WordPress / wp-movies-demo

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

Work with client side navigation disabled #15

Closed michalczaplinski closed 1 year ago

michalczaplinski commented 1 year ago

⚠️ Stacked PR (note the PR base)

This PR makes the demo work with the client side navigations disabled.

The missing bit was to include the missing wp_directives_mark_interactive_blocks function.

I also took this opportunity to:

SantosGuillamot commented 1 year ago

I've tested the UX, and it looks great! 🙂 A couple of comments:

michalczaplinski commented 1 year ago

When client-side navigation is disabled, I am not sure if we should trigger the navigation when the user stops typing or if we should wait for another input like "submitting the form" by clicking a button or pressing the Enter key.

You're right! This would be a better UX. Either that, or we should add the debouncing when the client-side navigations are disabled.

I will try adding a "search" button next to the input only when the client-side navigations are disabled.

Maybe we should keep the search query in the search box after navigating. Right now, when you search for the results, the search value is empty. As mentioned https://github.com/c4rl0sbr4v0/wp-movies-demo/pull/13#issuecomment-1427680051, we could define the state.search.value in the server, and maybe we could use the search param in the URL.

Yeah, that's a really good idea! I'll add this in a separate PR. I will first try to move the demo to https://github.com/WordPress/block-hydration-experiments so that I can use the latest code for the server-side store.

michalczaplinski commented 1 year ago

I will try adding a "search" button next to the input only when the client-side navigations are disabled.

I've realized that to make this work in the most idiomatic way possible, I need to change the behaviour of actions.search.update() action depending on whether the client-side navigations are enabled or not.

I can easily get this setting on the server using sth like:

$settings = get_option( 'wp_directives_plugin_settings' );
if ( $settings['client_side_navigation'] ) {
    // navigations are enabled
}

But if I want to get this setting on the client in the view.js file, I'd have to do something hacky like save it in the DOM on the server and read it out in JS on the client, yuck.

The most idiomatic way to do it is gonna be to to save the value of client_side_navigation in the store (on the server) using https://github.com/WordPress/block-hydration-experiments/pull/147.

I ll implement this feature once I've moved the demo to https://github.com/WordPress/block-hydration-experiments.

I'm now going to only merge the changes to the README in this PR. The rest of the changes in this PR were just pulling the latest updates from BHE and adding "supports": "interactivity" to the block.jsons which we can discard for now.

michalczaplinski commented 1 year ago

Actually, instead of figuring out which commits to undo, I ll just close this PR and add the README updates when I open the PR against BHE. That'll have to wait till tomorrow though :)

luisherranz commented 1 year ago

The most idiomatic way to do it is gonna be to to save the value of client_side_navigation in the store

We need to investigate if we want to expose some parts of the router in the store, like:

{
  state: {
    router: {
      url: "https://mysite.com/post",
    }
  },
  actions: {
    router: {
      navigate: async (url) => {
        // ...
      }
    }
  }
} 

That would be the perfect place for a flag like that.

@michalczaplinski, would you mind adding it to the decisions that need to be made? Thanks!! 🙂

michalczaplinski commented 1 year ago

Sure thing! I've added this as an item under State management in the BHE Tracking Issue. Should we mention it anywhere else?

luisherranz commented 1 year ago

Maybe we should open an issue?

michalczaplinski commented 1 year ago

ok, done! https://github.com/WordPress/block-hydration-experiments/issues/80#issuecomment-1440639731