Added the following features:

punxaphil / maxi-media-player

Media card for Home Assistant UI with a focus on managing multiple media players, but not excluding single player setups.

7 stars 1 forks source link

Added the following features: #14

Closed thlucas1 closed 2 months ago

thlucas1 commented 2 months ago

add editor settings to allow show / hide of player control buttons: repeat, shuffle, next track, previous track, and power.
fixed an issue with media-control-service setVolumeMute and volumeDefaultStep to account for only 1 player; prior to this, it was ignoring the request (mute, volume up / down) if only one player was active.
added power button to player-controls if current player state is off.
when getting the speaker list, return the main player name if no pre-defined groups are defined.

thlucas1 commented 2 months ago

Hi Phil - just wondering if you saw this pull request? Anything else you need from me to get these changes in? Thanks - Todd

punxaphil commented 2 months ago

Thanks for contributing! It's a bit big this pr. can you make one pr per feature? 🙏

thlucas1 commented 2 months ago

Are you talking about 5 separate PR's for each of the player control button show / hide functions, as well as 3 more PR's for the other 3 changes? That seems like a lot of extra work for essentially doing the same thing. I know it seems like a lot of changes, but most of them are due to adding the configuration show / hide logic for the player control buttons.

To be honest it would be a pain to break the branch up, not to mention having to re-test again. My GIT skills are not the greatest, and I am not sure how to break them up without just starting over and making the changes again one by one.

But if you are set on requiring individual PR's, then I will see what I can do.

Also, it appears the change you made for adding the power button in the volume.ts file is using the <sonos-ha-player element name instead of the <mxmp-ha-player element name used elsewhere. I assume this is due to you trying to keep the Sonos Card and Maxi Card in sync maybe?

punxaphil commented 2 months ago

Not really, but one pr per bullet in your description.

That's a bug, thanks for finding that!

punxaphil commented 2 months ago

Bug fixed: https://github.com/punxaphil/maxi-media-player/releases/tag/v1.4.1

punxaphil commented 2 months ago

It is very common to have one feature/fix per commit in the main branch, so I hope you can accept that.

punxaphil commented 2 months ago

You need to update your branch with the main, let me know if you have issues resolving the conflicts.

thlucas1 commented 2 months ago

At this point I am confused on how to do this in GIT, and am quickly getting frustrated with trying to split these up.

I am going to close this PR, remove the repository, refork it, and rea-apply my changes.

punxaphil commented 2 months ago

Thanks I appreciate that

thlucas1 commented 2 months ago

OK, I removed the maxi-media-player from my GitHub repositories, and re-forked from yours with the most current update. I then removed the local copy on my machine, and did a git clone to redownload the local copy. Fresh as a daisy.

I created a branch called featurePlayerControlsHideShow. I have made the changes for the feature request and tested them. The feature request is ready to commit.

Should I commit the changes to featurePlayerControlsHideShow branch? Or should I commit them to main?

Once committed, I can then create a PR for it, correct?

Please let me know, and thanks for your help.

punxaphil commented 2 months ago

Should I commit the changes to featurePlayerControlsHideShow branch?

This. Then make a PR from that to this repo