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

Add basic controls to the player #12

Merged
merged 7 commits into from
Dec 5, 2024

Conversation

christriants
Copy link
Contributor

This PR adds basic players controls, which addresses issue #7.

We're adding a play button, volume button and track selector, replacing the controls that had been sitting below player. Each control is now its own component, and we attempted to move some logic relevant to these new components out of watch.tsx and to the respective component files.

Screenshot 2024-11-28 at 12 30 56 PM

To test these changes:

  • The play button should resume audio (we will add play/pause functionality when feature: play/pause and running promise modified #9 is merged)
  • The volume button should handle toggling mute
  • The track selector button should open a menu with available tracks, and selecting a track should update the current video track

Additionally, I renamed usePlayer to player in watch.tsx. I think this is a positive change because:

  1. it's idiomatic to name signals after the value they represent (player, setPlayer)
  2. ‘use’ is commonly associated with React hooks
  3. it reads as a direct reference to the player

All feedback is welcome!

@englishm
Copy link
Owner

englishm commented Dec 3, 2024

It's really nice to see some UI elements here! Thanks!
I'm afraid I set up a merge conflict for you when I merged the play/pause PR. Sorry!

You may also be interested in some of the discussion on #4, in particular regarding what Web Components might allow us to do for the UI.

I would like to the move in the Web Component direction in the long run, but since that requires some refactoring work first, I'm happy to accept PRs that add UI elements like you have here in the meantime. Hopefully some parts of that work can be ported (or re-used in) to a media-chrome model.

@christriants
Copy link
Contributor Author

@englishm No problem at all! Conflicts are resolved, and play/pause functionality has been added to the play button, since the PR to add that functionality has been merged.

Thanks for sharing that discussion. I will keep this direction in mind as this project continues (and hopefully contribute, as well)!

Appreciate you taking a look at this one.

@christriants christriants force-pushed the basic-player-controls branch 2 times, most recently from bee8775 to 26ccd3e Compare December 5, 2024 04:32
@christriants christriants force-pushed the basic-player-controls branch from 26ccd3e to 2a5069b Compare December 5, 2024 04:36
@englishm
Copy link
Owner

englishm commented Dec 5, 2024

Tested, works!

One minor issue I notice is that sometimes there seems to be a brief delay in responding to clicks, but I think that may be unrelated to these changes and it isn't a major issue so I'm not going to worry about it right now. (I wonder if it might be related to how we're waiting on keyframes?)

Thanks for this, @christriants! I'll merge and then we can start looking at @JoaquinBCh's refactor and switch to rollup, etc. 😁

@englishm englishm merged commit bbc8db9 into englishm:main Dec 5, 2024
1 check passed
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.

2 participants