AmpliPi icon indicating copy to clipboard operation
AmpliPi copied to clipboard

PlayerCardSelection

Open SteveMicroNova opened this issue 1 year ago β€’ 2 comments

What does this change intend to accomplish?

Closes #529 How to close that issue is pending, presently this is a draft with the first idea that came to mind so we can talk about it

Checklist

  • [ ] Have you tested your changes and ensured they work?
  • [ ] Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • [ ] If applicable, have you updated the documentation/manual?
  • [ ] If applicable, have you updated the CHANGELOG?
  • [ ] Does your submission pass linting & tests? You can test on localhost using ./scripts/test
  • [ ] Have you written new tests for your core features/changes, as applicable?
  • [ ] If this is a UI change, have you tested it across multiple browser platforms?
  • [ ] If this is a UI change, have you tested across multiple viewport sizes (ie. desktop versus mobile)?

SteveMicroNova avatar Jun 28 '24 20:06 SteveMicroNova

One failure with this present implementation is that it doesn't work on mobile, which makes it a bit of a nonstarter imo but it was my initial thought, I've found it's better to come to the table with a baseline idea for people to say yes or no to rather than ask them to pick something out of the ether themselves when it comes to UI stuff.

https://github.com/micro-nova/AmpliPi/assets/114415937/3f620f9f-f5a1-487e-8e4f-5713a7a6a901

After having a short talk with Lincoln just now, his preference is to solve the other half of this issue instead, simply make one of the cards selected by default; I prefer the other version as making them obviously selectable will make it so that we don't have a "How do I change what the controls page is controlling" question in the future.

Though on the other hand, perhaps having one of them always selected and having the users notice that it is selected on the player page will lead to them realizing things are selectable in the first place, I feel like that's entirely possible within the user story that underpins this issue

SteveMicroNova avatar Jun 28 '24 20:06 SteveMicroNova

I think the hover functionality works great for desktop but it has no equivalent for mobile. My vote is to keep the hover but also select the first? card if none is selected.

linknum23 avatar Jul 01 '24 15:07 linknum23

The easiest way to test this, if you want to see it work, is have the same unit open in two windows and close the source that the other window has selected

Previously, that would lead to being sourceless; now it autoselects a different source (if available)

https://github.com/user-attachments/assets/980f4ee5-b2f9-44b0-812f-6bff8d13ede1

This was somewhat difficult to show in a concise video without a voiceover, hopefully me showing the two different units across 4 windows and the difference of what happens when you close a selected source gets the point across

SteveMicroNova avatar Jul 16 '24 14:07 SteveMicroNova

I've requested review from @linknum23 as well, as Klayton is out of office today

Lincoln, it's alright if you don't have time for this PR, I've primarily requested your review since you gave early thoughts on this PR and asked for a feature that was added along the way

SteveMicroNova avatar Jul 16 '24 14:07 SteveMicroNova

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 51.67%. Comparing base (b516bb7) to head (c68c25d). Report is 175 commits behind head on main.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #788      +/-   ##
==========================================
+ Coverage   50.90%   51.67%   +0.76%     
==========================================
  Files          25       26       +1     
  Lines        5838     6675     +837     
==========================================
+ Hits         2972     3449     +477     
- Misses       2866     3226     +360     
Flag Coverage Ξ”
unittests 51.67% <ΓΈ> (+0.76%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jul 16 '24 14:07 codecov-commenter

This branch should include an "autoselect new source" before merging; That is, if you hit the add button and create a source it should be selected for you automatically

SteveMicroNova avatar Jul 16 '24 15:07 SteveMicroNova