AmpliPi icon indicating copy to clipboard operation
AmpliPi copied to clipboard

SongInfoStyle

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

Add customizable style for SongInfo, right align metadata text on player card on Home page and limit the max size of metadata

What does this change intend to accomplish?

Metadata on player cards can take up a lot of space, reduce that max space and wrap text as necessary

See images in first comment for a diff

Checklist

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

SteveMicroNova avatar May 07 '24 13:05 SteveMicroNova

Old: image

New: image

SteveMicroNova avatar May 07 '24 14:05 SteveMicroNova

On mobile, note how much taller it is due to the width limiting. The intention is to keep the sources/stream data from fighting for space on mobile:

Old: IMG_2119

New: IMG_2120

SteveMicroNova avatar May 07 '24 14:05 SteveMicroNova

I'll also note: These are largely stylistic choices, I know the maxwidth thing assisted with some fighting for space but the right text align just looks better on player cards imo so you don't have a blob of text floating in the middle of the card

SteveMicroNova avatar May 07 '24 14:05 SteveMicroNova

Was testing on a unit capable of multi-zone play, found more bugs, fixed them I've made it so that the zone names are dynamically moved around to make space for the longer ones as needed to not cover or otherwise impact metadata Note that it is still possible to cover up metadata with a zone name, but only if you have a single extremely long word

Old: image

New: image

SteveMicroNova avatar May 07 '24 15:05 SteveMicroNova

Looking into ways of potentially limiting max size in previous screenshots

SteveMicroNova avatar May 07 '24 15:05 SteveMicroNova

Looking into ways of potentially limiting max size in previous screenshots

Figuring this out is nontrivial and has minimal dividends so I am no longer pursuing it

SteveMicroNova avatar May 07 '24 16:05 SteveMicroNova

what github issue does this address?

None to my knowledge, it was a change added in #703 that I added to attempt to prevent zones overlapping with metadata (generally the metadata is long, I tried and failed to do the same with a long zone)

not a huge fan of the wrapping change, and I dont' think you've necessarily made the case that a) this is a problem we should spend time solving, and b) this is the best solution to that problem. making these boxes variable height is the quickest way to make this look messy imo.

I'm not sure I like the vertical resizing either, that was just the easiest way to go about fixing this. I could technically also do dynamic font scaling, but that sounded messy

SteveMicroNova avatar May 07 '24 17:05 SteveMicroNova

New version, now with ellipsis wrapping (based onto #710 to avoid making the same changes to Chip twice)

image

SteveMicroNova avatar May 07 '24 19:05 SteveMicroNova

Codecov Report

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

Project coverage is 49.51%. Comparing base (b516bb7) to head (a296d1a). Report is 58 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     #708      +/-   ##
==========================================
- Coverage   50.90%   49.51%   -1.39%     
==========================================
  Files          25       26       +1     
  Lines        5838     6349     +511     
==========================================
+ Hits         2972     3144     +172     
- Misses       2866     3205     +339     
Flag Coverage Ξ”
unittests 49.51% <ΓΈ> (-1.39%) :arrow_down:

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 May 07 '24 21:05 codecov-commenter

is the original SongInfo.jsx still in use, or can it be removed?

The original Songinfo is still in use on the player page

I'm still hesitant to approve this, because I see no less than 5 useStates that can cause re-renders and a useEffect that watches 3 of them. Is there any possible way to reduce the amount of state stored and reacted upon?

State could potentially be managed better, can't say I know how as everything here makes sense to me. I could maybe cut out the useEffect and put it on some sort of regular check timer but I don't think that's actually an improvement

SteveMicroNova avatar May 15 '24 12:05 SteveMicroNova

This is leaking off the rightmost edge of playercards atm, needs more work but I have no opportunity to dig that deep atm

SteveMicroNova avatar May 15 '24 21:05 SteveMicroNova

what other solutions have you examined for this problem?

I answered this in a slack DM, but to answer it publicly as well: I've tried doing it myself in CSS and that proved to have far too many levers that were all extremely sensitive to any little change and it made the animation look very chunky and didn't fully scroll like I wanted to (instead snapping back to the start position when the text was fully offscreen rather than wrapping back around to come to a stop), so this was the only alternative that I could find The library I'm using is depended on by 34k+ repos so I had the feeling it was a good route to go down even if it lacks a small amount of tooling that I've managed to create with that state handling I can pare down the state a little more yet, that windowSize ref is uneccessary and was only there because I blindly turned almost all states into refs instead when it can realistically be just the window.innerWidth at all times

SteveMicroNova avatar May 17 '24 13:05 SteveMicroNova