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

Work with client side navigation disabled #15

Closed
wants to merge 5 commits into from

Conversation

michalczaplinski
Copy link
Collaborator

@michalczaplinski michalczaplinski commented Feb 10, 2023

⚠️ 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:

  • Rename all references to "client side transitions" with "client side navigations"
  • Remove the client-side-transitions.php file and move the code into init.php
  • Update the runtime to the latest version from the block-hydration-experiments

- Update to the latest version of the library
- Add the missing code that adds wp-island
- Update to the latest version of the library
- Add the missing code that adds wp-island
@michalczaplinski michalczaplinski changed the base branch from main to move-view-js February 10, 2023 23:47
@michalczaplinski michalczaplinski changed the title Work-with-no-transitions Work with client side navigation disabled Feb 10, 2023
@michalczaplinski michalczaplinski mentioned this pull request Feb 10, 2023
3 tasks
@SantosGuillamot
Copy link
Collaborator

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

  • 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.
  • 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 here, we could define the state.search.value in the server, and maybe we could use the search param in the URL.

"style": "file:./style-index.css"
"style": "file:./style-index.css",
"supports": {
"interactivity": true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this to make the block interactive? If so, why? I thought this property wasn't needed anymore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have to add this property so that the wp_directives_mark_interactive_blocks function can add the wp-island to the interactive blocks.

Alternatively, the user can add the wp-island attribute manually in the render.php function but I think we should encourage adding this in the block.json.

There is a discussion around 1) detecting the interactive (using directives) blocks automatically in:

But we haven't settled on a definite solution yet as far as I can see.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right, I forgot about that 😅 . Hopefully, we can remove it in the future once that is done automatically.

@michalczaplinski
Copy link
Collaborator Author

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.

Base automatically changed from move-view-js to main February 15, 2023 20:05
@michalczaplinski
Copy link
Collaborator Author

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 WordPress/block-interactivity-experiments#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
Copy link
Collaborator Author

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
Copy link
Member

luisherranz commented Feb 16, 2023

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
Copy link
Collaborator Author

michalczaplinski commented Feb 22, 2023

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

@luisherranz
Copy link
Member

Maybe we should open an issue?

@michalczaplinski
Copy link
Collaborator Author

ok, done! WordPress/block-interactivity-experiments#80 (comment)

@cbravobernal cbravobernal deleted the work-with-no-transitions branch October 11, 2024 07:29
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.

4 participants