WordPress / wp-movies-demo

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

Show more data on movie pages #19

Closed SantosGuillamot closed 1 year ago

SantosGuillamot commented 1 year ago

This PR is based on cron-db-update branch because it includes more custom fields than the main branch. I'm using it to explore alternatives, I assume some parts won't be compatible with the main branch, as it is missing some data right now.

Based on TMDB Templates, I started this Pull Request to add more data to our movie pages. I'm just exploring alternatives, and later we can decide which functionalities we should include or not.

This is an example of how it is going so far:

Screenshot 2023-02-16 at 09 08 05

What I've done:

SantosGuillamot commented 1 year ago

I came up with an approach to play the trailers and keep the video playing while navigating. It also updates whenever another trailer is played and it can be closed whenever users want. I made a quick video explaining everything better.

I am not 100% convinced of the approach, so I'd love to hear your thoughts. At least, it seems feasible 😄

https://www.loom.com/share/c7e43d7d7bbc439f88c59244fe593290

SantosGuillamot commented 1 year ago

I added a new interactive block that lets users toggle between images and videos of the movie. I recorded a quick video showing how it works:

https://www.loom.com/share/fa39905a266d456e9c1de82516418921

As explained in this issue, right now, this won't work after navigating from the homepage. You need to load the movie page directly.

Now that the initial functionalities that I had in mind are covered, I'd like to polish the code and the styles.

SantosGuillamot commented 1 year ago

The changes you made in the latest commits look great Michal! Thanks for that 🙂

Not related to that, I have one question regarding the Search template. Is it doing something that the Archive template is not? I mean, could we use the Archive template for the search or is there anything missing?

If we just remove the current search template, we could reuse the styles from the Archive.

With the Search template:

Screenshot 2023-02-24 at 13 06 49

Without the Search template (using Archive):

Screenshot 2023-02-24 at 13 04 29
SantosGuillamot commented 1 year ago

The pagination numbers and footer links looked a bit weird to me without the underline. I added some CSS to add it again. The remaining links should remain unaltered. Hope that's okay.

Before:

Screenshot 2023-02-24 at 13 32 53

After:

Screenshot 2023-02-24 at 13 32 03
michalczaplinski commented 1 year ago

Not related to that, I have one question regarding the Search template. Is it doing something that the Archive template is not? I mean, could we use the Archive template for the search or is there anything missing?

Yes, there's nothing special about the Search template! It was mostly a stylistic choice because I wanted the search UI to look a bit distinct from the UI for the movies page but . My main reason for keeping it though would be that we got feedback from Greg & Matias that it was very illuminating to see that you can delete the Search template and the system falls back on the Archive template. So I wanted to add a note in the README under "things to try" that you can remove the Search template and the system will fall back on the Archive.

The pagination numbers and footer links looked a bit weird to me without the underline. I added some CSS to add it again. The remaining links should remain unaltered. Hope that's okay.

Good catch, looks better! 👍

michalczaplinski commented 1 year ago

There's one more thing that I'd like to add in this PR:

Only show the "Close" button of the video player if the user is hovering over the player - this is the behavior on desktop. On mobile - show the close button only if the user has tapped on the video and hide it if the user has tapped outside of the video.

I think this is gonna be a nice example of a custom directive 🙂 .

michalczaplinski commented 1 year ago

Only show the "Close" button of the video player if the user is hovering over the player - this is the behavior on desktop. On mobile - show the close button only if the user has tapped on the video and hide it if the user has tapped outside of the video.

I've started prototyping this but I've realized that it's probably not the best UX after all. On mobile, the user would have to tap on the video to show the "Close" button. But at the same time tapping the video should pause the video. So, we'd have to add an overlay over the video that captures the first tap and only the second tap should actually pause the video... That's getting a bit complicated for little benefit really 😅 .

SantosGuillamot commented 1 year ago

My main reason for keeping it though would be that we got feedback from Greg & Matias that it was very illuminating to see that you can delete the Search template and the system falls back on the Archive template. So I wanted to add a note in the README under "things to try" that you can remove the Search template and the system will fall back on the Archive.

That totally makes sense! I hadn't thought of that 🙂

That's getting a bit complicated for little benefit really 😅 .

I will probably leave it as it is right now then. I believe it's already looking nice!

michalczaplinski commented 1 year ago

I will merge it now 🙂

The video player and the tabs are still broken when the client-side navigations are disabled but should work when we merge the demo into https://github.com/WordPress/block-hydration-experiments