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

Player, Player Card synchronization #756

Merged
merged 1 commit into from
Jun 19, 2024
Merged

Conversation

SteveMicroNova
Copy link
Contributor

@SteveMicroNova SteveMicroNova commented Jun 17, 2024

What does this change intend to accomplish?

Adds marquee to player page, getting rid of font scaling issue Ryan and I found while I was showing off #747 related to using the same .scss file for both SongInfo and SongInfoMarquee

I've added the marquee to the player page, and as such SongInfoMarquee has fully replaced the original SongInfo and taken its name

I've also added MUI Grid support to the player card and player page as a fast method of using CSS grid styling to ensure that metadata always starts and ends in the same spots, rather than being a dynamically sized mess (see images below)

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?
  • Does your submission pass linting & tests? You can test on localhost using ./scripts/test
  • 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
Copy link
Contributor Author

Old:
image
image

New:
image
image

Note that the new stuff is the marquee and so doesn't fit, but does scroll

The intended fix was the font size of the home page player cards on smaller screens, synchronizing the marquee to the player page as well (cutting down on total number of components, removing text wrapping that increases page height) was just a bonus

@SteveMicroNova SteveMicroNova marked this pull request as draft June 17, 2024 19:55
@SteveMicroNova SteveMicroNova marked this pull request as ready for review June 18, 2024 19:03
Comment on lines 73 to 96
<Grid
container
direction="row"
justifyContent="center"
alignItems="center"
>
<Grid item xs={4} md={3}>
{ !is_streamer && (
<div className="zones">
<ZonesBadge sourceId={sourceId} onClick={openZones} />
</div>
)}
{ is_streamer && (
<div className="streamer-outputs">
<StreamerOutputBadge sourceId={sourceId} />
</div>
)}
</Grid>
<Grid item xs={4} md={6}> {/* Spacer */} </Grid>
<Grid item xs={4} md={3}>
<SongInfo sourceId={sourceId}/>
</Grid>

</Grid>
Copy link
Contributor Author

@SteveMicroNova SteveMicroNova Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added support for the MUI Grid component, which is a fast way of implementing CSS grid formatting support. It has built in breakpoints (xs, sm, md, lg, xl) for different screen sizes, then you identify what relative size you want everything to be, interpreted as a percentage of adding all the same breakpoints up together

I always have my breakpoints add up to 12 as that's the tutorial on the MUI website, though that's arbitrary. In this case, at the xs breakpoint everything is sized at 4/12 (33%) and at md and larger it's sized [3/12, 6/12, 3/12] [25%, 50%, 25%]

The primary reason I went to grid formatting is this:

Old:
image

New:
image

Wanting everything to have a properly preset area rather than the metadata taking up double the width of the widest portion (or whatever it was actually doing) due to how the inherent Marquee styling works

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've since changed these particular breakpoints a little, but the underlying explanation is still good for those who wish to know how the Grid breakpoints work

@SteveMicroNova SteveMicroNova linked an issue Jun 18, 2024 that may be closed by this pull request
Copy link
Contributor

@rtertiaer rtertiaer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one change in the CHANGELOG but otherwise looks good to me. just to be explicit - you've tested this latest grid + breakpoint setup on different viewports/platforms yea?

CHANGELOG.md Outdated
@@ -12,6 +12,7 @@
* Make update available badge show up on both Settings page and the menu bar
* Fix stream icons
* Make it clearer when a stream needs a zone added on the home screen
* Enforce breakpoint styling to ensure that the UI looks the same between mobile, desktop, tablet viewports
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you rebase this PR and put this under a Next Release heading?

@SteveMicroNova
Copy link
Contributor Author

just to be explicit - you've tested this latest grid + breakpoint setup on different viewports/platforms yea?

Chrome, Firefox, and mobile safari yes
Chrome and Firefox both got window resizing treatments during testing, I can't do that on mobile safari but it seemed fine

Add documentation

Add breakpoint spacing to player card, player page to ensure consistent styling at all screen sizes and metadata lengths

Update CHANGELOG

Smooth out breakpoints on player page

Smooth playercard breakpoints

Update Changelog
@SteveMicroNova SteveMicroNova merged commit 8c10ec7 into main Jun 19, 2024
3 checks passed
@SteveMicroNova SteveMicroNova deleted the PlayerFontSizeFix branch June 19, 2024 16:37
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

Successfully merging this pull request may close these issues.

Inconsistent metadata positioning
2 participants