Metadata Marquee

[SteveMicroNova]

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?

Add Marquee to metadata on screens where metadata doesn't fit properly

Ignore the images in the chat below, I was also trying to do text wrapping and other things along the way

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]

Old:

New:

[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:

New:

[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]

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:

New:

[SteveMicroNova]

Looking into ways of potentially limiting max size in previous screenshots

[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

[rtertiaer]

what github issue does this address?

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.

[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]

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

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.51%. Comparing base (b516bb7) to head (21d0f3b).
Report is 54 commits behind head on main.

❗ 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%)	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rtertiaer]

what does this accomplish?

[SteveMicroNova]

I'm unclear as to why we do a += to the classnames, that was code that already existed in the original SongInfo that this was forked off of
The useRefs are just a React approved way of doing something along the lines of document.getElementById()

[rtertiaer]

where do all the ref values come from? I see you set them to null here; where are they populated?

what is the purpose of this line?

  artistClassName = "artist-name " + artistClassName;

[SteveMicroNova]

return (
        <div className="song-info song-info-marquee">
            <div ref={artistRef} className={artistClassName}>
                <CustomMarquee data={info.artist} containerRef={artistRef} />
            </div>

            <div ref={albumRef} className={albumClassName}>
                <CustomMarquee data={info.album} containerRef={albumRef} />
            </div>

            <div ref={trackRef} className={trackClassName}>
                <CustomMarquee data={info.track} containerRef={trackRef} />
            </div>
        </div>
    );

The refs are populated by the elements being assigned a given ref

[SteveMicroNova]

As for the ClassNames, @klay2000 will have to answer for that as he (or Jonah) created those. I assume they're a way of passing external classes in while also defining a baseline class for each portion, though I don't know if there's any classes that are ever passed in in practice

[klay2000]

I believe this was necessary to re-use song info on the stream card and the player view

[SteveMicroNova]

With this clarification I have removed them as this PR splits SongInfo.jsx into two files, one for the home screen player cards and another for the player view metadata

[rtertiaer]

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

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?

[SteveMicroNova]

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]

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

[rtertiaer]

where do you set this?

[SteveMicroNova]

I'm using refs as a document.getElementById, which you can't use directly because react elements are too transient to have an ID at all times
https://react.dev/learn/manipulating-the-dom-with-refs#getting-a-ref-to-the-node

You send a null useRef to the value of a ref prop on an element and it magics it together to be a functional equivalent to document.getElementById for me to read the innerWidth of

[rtertiaer]

where do you set this tho? i don't see a dataRef.current = [...] anywhere

[SteveMicroNova]

I suppose that link wasn't a perfect example

You don't need to set the value of current directly at any point in some cases, this being one of them

I set the value of that ref in the div inside of the Marquee, and I use that to measure the width of the content vs the width of the marquee to decide if to should or shouldn't scroll

        <Marquee play={dataScroll} speed={30} onCycleComplete={() => {setOverride()}}>
            <div
                style={{marginLeft: "10px"}}
                className="marquee-text"
                ref={dataRef} // Here
            >
                {data}
            </div>
        </Marquee>

And then during assessMarquee(), I set the value of a const to the value of dataRef.current so I can use that ref as a reference to the element and measure the relative widths

[rtertiaer]

what other solutions have you examined for this problem? i feel like you haven't convinced me that, out of the entire solution space, doing this in javascript, importing a library, and wrapping it ourselves with a ton of state is the correct solution.

[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

[rtertiaer]

i'd like to be a bit more rigorous here. there are a couple of unhandled cases:

• container.offsetWidth < scroll.scrollWidth && dataScrollOverride.current
• container.offsetWidth >= scroll.scrollWidth && !dataScrollOverride.current

i see here that the data/control flows thru two different paths:

• one with setDataScroll(): you call setOverride() thru onCycleComplete(), which sets dataScrollOverride
• and another with clearTimeout(), which cancels dataOverrideTimeout and thus never sets dataScrollOverride.current = false

is there any way you can either shore up this conditional to handle the other cases, or flatten the control flow such that the actions triggered by this conditional aren't behind a couple of layers of indirection that need to be correctly executed to function the next time assetssMarquee() is called?

[SteveMicroNova]

I feel that those unhandled cases can remain unhandled, and simply continue to do whatever it was already doing do to how dataScrollOverride is handled:
dataScrollOverride is set to true at the end of every marquee cycle, and then reassesses whether it should be true or false after a two second delay. If it is left true then any event that could turn it false triggers a reassessment of the state
I would move the dataScrollOverride check into the play state of the Marquee element to make reading easier, but doing so makes it so that making the screen wider suddenly can stop the marquee in the wrong spot as soon as the screen is wide enough to fit it.

As for the data flow concern, I don't think it's that bad? There's a binary state: playing or not. That relies on two other binaries, one being "does the info fit inside the space given to it" and the other is "is the scrolling paused", if the scrolling is paused it gets unpaused after 2 seconds max, or doesn't unpause if the screen is now wide enough to fit the content. It's done this way to ensure the content always stops in the correct position, getting rid of the dataScrollOverride and instead simply using the playing state would cause the text to stop in weird spots instead of having a well defined stopping point and not having timers makes the data scroll constantly, which looks bad.

I feel like graphing the flow out would make it look nicer / less complicated, but I'm not doing that unless I'm at the whiteboard and I'm not able to spend the time to do that today.