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

MediaPlayerCard improvements #91

Closed
3 tasks done
yann510 opened this issue Oct 31, 2023 · 22 comments
Closed
3 tasks done

MediaPlayerCard improvements #91

yann510 opened this issue Oct 31, 2023 · 22 comments

Comments

@yann510
Copy link
Collaborator

yann510 commented Oct 31, 2023

  • Add a modal or dropdown to select which speakers should be part of the group
  • Fix artworkUrl as in my case it doesn't return the full URL, I need to append the home assistant URL to it
  • Display all grouped speakers as the friendly entity name

The goal for me would be that this card can act as the single controller for all of my media entities.

@shannonhochkins I'd love to hear your thoughts on this, especially the first point :)
If you have other ideas, I'm very open to them.

@shannonhochkins
Copy link
Owner

I'll take a look! If the entity doesn't have a group_members property the players won't be in sync, but I can at least provide a secondary attribute to just play it through multiple regardless if they're in sync or not?

Does your media_player have the group_members property in the attribute?

As for the artwork, that's an oversight on my behalf, I tested with streaming services but didn't think about local media, easy fix

As for the friendly name, it should already be doing this if the friendly name exists in home assistant, are you seeing otherwise?

@shannonhochkins
Copy link
Owner

I will fix the artwork url with the next release, I'm doing a massive refactor and improvements on responsive layouts at the moment to give more control to the user and to cleanup alot of repetitiveness, will wait for you to clarify the last point

@yann510
Copy link
Collaborator Author

yann510 commented Nov 1, 2023

I've already started working on this, I should have a pr for this shortly :)

For the friendly name it doesn't show the name of all grouped speakers, just the master one. So I'm making sure to display the name of all grouped entities.

@yann510
Copy link
Collaborator Author

yann510 commented Nov 2, 2023

Hey @shannonhochkins

This is very much a POC, but I'd like to get your initial thoughts on the UI/UX.

chrome-capture-2023-10-2

I was also thinking that maybe it would be nice to avoid the modal altogether and use chips components to select/unselect grouped speakers.

@shannonhochkins
Copy link
Owner

Hi yann, thanks for asking for feedback, and I think you're right, I think this behaviour should be quick access and easy to manipulate

What I suggest, is potentially a collapsible section within the card where you can not only enable the players, but also control volume of individual players, would you like me to mock something up for you?

Great work though, it's nice to see you're interest in the project and that you've managed to pickup what you've picked up with no guidance!

@yann510
Copy link
Collaborator Author

yann510 commented Nov 2, 2023

I would absolutely love some help with a mockup, you have great UI/UX skills ;)

@shannonhochkins
Copy link
Owner

No problems mate! I will send something over when I can, things are a bit wild over here with work and in general, but I'll try send you something tomorrow :)

How have you found working with the kit in general, any feedback? It is a bit of a learning curve for sure

@yann510
Copy link
Collaborator Author

yann510 commented Nov 2, 2023

I get it; life can be crazy at times!

You've done an outstanding job with this framework. I do have a few points of feedback for newcomers like me:

  • Write smaller files: some files have over 800 lines, which makes it really hard to understand what's going on. We should split these into smaller chunks for readability.
  • Code style: this is more a personal preference, but I find the 80-character limit of prettier quite hard to work with. I've been working with 140 for the past few years, and it is a life changer. It really cleans up the code by avoiding multiple lines on small blocks of code.
  • Documentation: has been beneficial. It's straightforward to grasp the different components available simply by using storybook.

@shannonhochkins
Copy link
Owner

Agree on all your points and I'm fine with the 140 character limit, I work on a 40" wide monitor so I'm sure I'll be fine with that change to character limits haha

Didn't even realise some of the files were that long, it's generally a rule I follow, and usually something I enforce in my team at work too (guess that's what happens when you fly solo and move too quickly 😉)

As for the documentation, that's good to hear! And I've actually made the docs better again in the next release (story book args table was missing plenty of props because of typescript alias issues)

But thankyou! All very valid points!

@yann510
Copy link
Collaborator Author

yann510 commented Nov 2, 2023

Thanks for the reply and I must say, It's fun to work with someone as passionate as you!

@shannonhochkins
Copy link
Owner

Sorry, i've been working on the massive pr: #92, haven't yet reviewed your PR but will do soon, and i'll get you a mockup of tthe media player as soon as i can!

@shannonhochkins
Copy link
Owner

shannonhochkins commented Nov 3, 2023

Regarding the media player ui, here's what i suggest!

  1. In the code, there's already a button with a class of "speaker-group", which is conditionally rendered based on the suppport of grouping
  2. There's an error that's thrown if a user provides the groupMembers prop but the player doesn't support grouping, remove that error, and instead the joinGroups method should be conditionally called based on the support
  3. Now that this button is rendering, in the onClick of this button, create a new state variable called "showGrops" or something similar
  4. When showGroups is true, hide the entire "Base" element, and render the VolumeControls component in the "slider" layout for every group_members property, or use the allEntitiesId

As for your idea around selecting which players should part of the group, I think by simply including a mute button for each entity is enough!

The idea here is that you can basically swap the view inside the card to control the volume / power / mute etc from within the card, i think add a simple divider between each entity and it should be okay!

If you get to a point with the functionality and you're not overly happy with the UI i can do some tidy up for you!

@yann510
Copy link
Collaborator Author

yann510 commented Nov 3, 2023

If you have time, I think it would be worthwhile for us to hop on a call and discuss these, just to make we understand each other and we're on the same page as to what we're trying to achieve.
My discord id is: king_luche

@shannonhochkins
Copy link
Owner

shannonhochkins commented Nov 3, 2023

That's cool bro, might be easier chatting there anyways!

@yann510
Copy link
Collaborator Author

yann510 commented Nov 4, 2023

Still very rhough in terms of it fully working and being polished, but what do you think about this design?
chrome-capture-2023-10-4

@shannonhochkins
Copy link
Owner

That's cool bro! I know it's not something you've done, but the flicking animation is a bit odd after pressing the enable buttons! I'll have to see what has happened there because it's happening in other places too

As for the ui I think it's fine! You've done a good job 👏

@yann510
Copy link
Collaborator Author

yann510 commented Nov 6, 2023

Yeah, there are quite a few challenges to solve, one of them being that sometimes it seems the state isn't correctly propagated on state changes, but when I refresh the page, everything is fine.
Have you ever had this issue?

@shannonhochkins
Copy link
Owner

shannonhochkins commented Nov 6, 2023

Yep! You have two options, remove internal state and just use the property on the entity which will basically cause a slight delay in the UI, or you can do an optimistic update, (set state, trigger service call, use useEffect watching the entity values, then update state from the changed values

If you want to do this without the optimistic hook, you can simply do something like

const entity = useEntity(_entity);
const [groupMembers, setGroupMembers] = useState([]);

useEffect(() => {
    setGroupMembers(entity.attributes.group_members);
}, [entity.attributes.group_members]);

return <button onClick={() => {
    /// logic to group members, set internal state here
}}>join</button>

Im assuming this is the issue you're facing, you do need to create a sync between the state and the entity, in some components I don't have internal state at all but rely completely on the entity values updating via websockets, this is usually fine as the updates are fairly quick and you don't notice too much latency

@yann510
Copy link
Collaborator Author

yann510 commented Nov 7, 2023

Awesome. Thanks for the detailed reply, you really are aware of all the new toys that come out, and I thought I was keeping myself up to date 😂

I'll do some more investigation this week and come out with a nice solution.
To get the state the update, do you use the redux store or get it from Hass entities usually?

@shannonhochkins
Copy link
Owner

Haha, we have to keep up. In our world 😅 the front-end world changes so quickly that it's insane!

I use zustand for the store internally, I find zustand to be the simplest manager out there, currently the store isn't really use for anything other than globally shared data

If you need to jump on another call to go over the issue I'm happy to help :)

I'm about to start focusing on an addon for hakit, my end goal is to have templates people can purchase with drag / drop / resize

What's your thoughts on this? Do you think people would purchase templates? They'd still be highly customisable, curious to get your thoughts

@yann510
Copy link
Collaborator Author

yann510 commented Nov 7, 2023

If you need to jump on another call to go over the issue I'm happy to help :)

Sounds good mate, I'll let you know how if I need the help from the master ;)

I'm about to start focusing on an addon for hakit, my end goal is to have templates people can purchase with drag / drop / resize
What's your thoughts on this? Do you think people would purchase templates? They'd still be highly customisable, curious to get your thoughts

I think that's a great idea, if I wasn't a freak of customizability I would definitely pay for such a beautiful UI/UX!!

@shannonhochkins
Copy link
Owner

Released under V3.0.4 - Thankyou for all your hard work :)

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

No branches or pull requests

2 participants