-
Notifications
You must be signed in to change notification settings - Fork 42
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
Improve code and docs #67
Conversation
- `/lib` - Helpers and support code for this demo. It's not relevant for | ||
understanding the Interactivity API. | ||
**Follow this guide to understand how we built each interactive block with the Interactivity API:\ | ||
[Movie Interactive Blocks](https://github.com/WordPress/wp-movies-demo/blob/main/docs/interactive-blocks/README.md)** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I included a link so people can find the documentation for the blocks.
@@ -46,30 +47,20 @@ The Interactivity API is [available in WordPress 6.5](https://make.wordpress.org | |||
npm run build | |||
``` | |||
|
|||
If you plan on tinkering with the frontend code, start the webpack | |||
server which automatically rebuilds the files when you make any changes: | |||
If you plan on tinkering with the frontend code, start the webpack server which automatically rebuilds the files when you make any changes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In many places, I've removed line breaks to allow markdown to handle line breaks wherever it's convenient based on the user's screen.
|
||
```sh | ||
npx wp-env run cli "wp theme activate wp-movies-theme" | ||
npx wp-env run cli -- wp theme activate wp-movies-theme |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using double quotes to pass the command doesn't work or has stopped working. More info in this issue.
npx wp-env run cli -- wp plugin activate wordpress-importer | ||
npx wp-env run cli -- wp import wp-content/plugins/wp-movies-demo/wp_sampledata_movies.xml --authors=create | ||
npx wp-env run cli -- wp import wp-content/plugins/wp-movies-demo/wp_sampledata_media.xml --authors=create | ||
npx wp-env run cli -- wp import wp-content/plugins/wp-movies-demo/wp_sampledata_actors.xml --authors=create |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same here. I have also added the activation of the WordPress Importer plugin, which is needed to perform the imports.
</div> | ||
``` | ||
|
||
We create an initial array to save the IDs of the favorite movies. | ||
|
||
_For this demo, we are not saving that data in the database, but if we were, that array would be populated with the list of favorite movies of a logged-in user._ | ||
|
||
In the `render.php` file, we populate the store with the array and the initial state of a couple of selectors which tell us the number of favorite movies and a boolean that tells us if there is at least one favorite movie. | ||
In the `render.php` file, we populate the state with the array and the initial state of a derived state which tell us if there is at least one favorite movie (`state.isLikedMoviesNotEmpty`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed all the references of selectors to derived state.
state.likesCount = state.likedMovies.length; | ||
state.isLikedMoviesNotEmpty = state.likedMovies.length > 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where the state was being calculated instead of derived.
state.likesCount = state.likedMovies.length; | ||
state.isLikedMoviesNotEmpty = state.likedMovies.length > 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as well, that also means that the logic was duplicated.
<form> | ||
<label class="search-label" for="movie-search">Search for a movie</label> | ||
<label class="search-label" for="movie-search"> | ||
<?php _e( 'Search for a movie', 'wp-movies-demo' ); ?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing translation.
const { actions } = await import("@wordpress/interactivity-router"); | ||
await actions.navigate(`/${url.search}${url.hash}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if it's not necessary in this case, it's good to teach where the store of the Interactivity router comes from and how to import it.
const { actions } = yield import("@wordpress/interactivity-router"); | ||
yield actions.navigate("/"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
> [!NOTE] | ||
> When running `wp-env` commands like the one above, it is assumed that the | ||
> Docker container is called `cli` on your local machine. If you had | ||
> previously used `wp-env` to run other WordPress sites, this name might be | ||
> different, e.g. `cli-1` in which case the full command would be `npx wp-env run cli-1 "wp theme activate wp-movies-theme"`. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deleted this note because it doesn't seem to be true. Even when the container is called cli-1
, wp-env
expects you to use only cli
. If you use cli-1
, it gives you the following error message:
Invalid values:
Argument: container, Given: "cli-1", Choices: "mysql", "tests-mysql", "wordpress", "tests-wordpress", "cli", "tests-cli"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was the one that added this note. I think that it was the case at the time but it was a while ago and I'm not sure how to intentionally reproduce it now. Happy to remove 🙂 .
02fed0a
to
d5b6205
Compare
I have created another pull request to prettify all the JavaScript files because we were not using the WordPress coding standard. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet, thanks for the updates Luis!
I ran through the updated setup instructions and it works like a charm.
Nice, thanks Michal 🙂 By the way, once this pull request is merged, how do you update the website? I think I've never done it before. |
Usually I generate the zips in local and just upload them to the site. We can enable Github deployments, but we may need additional permissions for the WordPress Github group. |
I'll try it. Thanks, Carlos! |
Plugin updated 🙂 |
What
I've been looking through the code and have improved a few things. I have also improved the README files.
Why
Some things were still outdated and did not correspond to how the Interactivity API works in its latest version. Other things are simply improvements.
How
I explained the changes in the comments below.