TrueKuehli / PruneTree

App for creating Sims 4 family trees. Fork of the Plum Tree app using local browser storage to replace API calls.
https://prunetree.app/
ISC License
3 stars 2 forks source link

Added a VideoPlayer component with placeholder for the video #48

Closed jCabala closed 9 months ago

jCabala commented 9 months ago

:sparkles: Added a VideoPlayer component with placeholder for the video

:books: Description

Moved the video player to the separate component and added a intitial placeholder with a button. If clicked, the video is shown and a entry in local storage is set to remember the user's choice

Closes #47

:construction: Remaining Work (optional)

[Any notes or related or other remaining work to be done]

:notebook: Notes (optional)

[Any additional notes not already mentioned]

:hammer: Testing

[Any manual test steps not covered by CI pipeline]

:microscope: Reviewing

:white_check_mark: Required Review Steps

  1. [] Check latest pipeline passes
  2. [] Code is well constructed (sensible implementation, location and documentation)
  3. [] Test coverage is acceptable
TrueKuehli commented 9 months ago

Looks great, thank you for your work! I have one small question, is there a reason you chose useLayoutEffect over useEffect?

jCabala commented 9 months ago

Since the default state is false if we use useEffect there is a small chance that the placeholder is going to load for a second even if the storage entry is set.

TrueKuehli commented 9 months ago

As far as I understand, useLayoutEffect blocks rendering though, right? Since the placeholder already has the correct dimensions, I feel like it's preferable to render that, instead of having the entire Home page wait for the hook to finish. Though admittedly it probably doesn't make a difference either way, so I'm sorry if I'm sounding nitpicky 😅

One thing I would definitely change though is the button text to make it more explicit that the video is hosted on YouTube (e.g. "Allow YouTube" or "Allow YouTube Player") since the placeholder is meant to prevent users from accidentally loading external content when they may not want to get tracked by YouTube. Once you make that change I'm ready to merge though!

jCabala commented 9 months ago

You're probably right, I'll chenge the useLayout effect and the text right away.

TrueKuehli commented 9 months ago

Awesome, thank you again for your contribution and for your patience with my change requests! I'll go ahead and merge the PR and push the new version online. I'm also planning on adding a proper way to credit you (and other contributors) in the app itself, which I've added a new issue (#49) for. You can let me know there if you think this is a good way to credit you or if you have any other ideas.