Skip to content

Conversation

@breville
Copy link
Member

@breville breville commented Dec 3, 2025

This makes a small adjustment to avoid the vertical overflow of timeline elements.

before

Screenshot 2025-12-02 at 9 57 50 PM

after

Screenshot 2025-12-02 at 9 56 44 PM

@breville breville requested a review from a team as a code owner December 3, 2025 01:14
@breville breville changed the base branch from staging-next to staging December 3, 2025 01:14
@breville breville requested a review from a team December 3, 2025 01:14
@breville breville requested review from dju90 and removed request for a team December 3, 2025 01:16
@breville breville changed the title Music timeline fix Music: timeline fix Dec 3, 2025
Copy link
Contributor

@hannahbergam hannahbergam left a comment

Choose a reason for hiding this comment

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

LGTM! Is it worth a brief code comment explaining the magic number 2?

@breville
Copy link
Member Author

breville commented Dec 3, 2025

LGTM! Is it worth a brief code comment explaining the magic number 2?

@hannahbergam I'm glad you asked, because I didn't have a great answer beyond it working for this fairly obvious case.

We work out the event height here, and part of that height is a gap at the bottom, calculated here. It turns out that with recent layout changes, our bar height is 132px but we actually get 127px to render these events.

I wrote a little code to output each row count, and its respective remaining vertical space, with negative space implying an overflow. I then adjusted the bar height down until we didn't get any overflow.

for (var rowCount = 5; rowCount <= 45; rowCount++) { 
  let eventHeight = Math.floor(128 / rowCount); 
  let gap = eventHeight > 8 ? 3 : eventHeight > 6 ? 2 : 1; 
  console.log(rowCount + ':', 127 - (eventHeight * rowCount - gap))  
}

It turns out that compensating by 4 pixels (i.e. assuming the bar height is 128px) was the magic number, so I've updated that.

@hannahbergam
Copy link
Contributor

LGTM! Is it worth a brief code comment explaining the magic number 2?

@hannahbergam I'm glad you asked, because I didn't have a great answer beyond it working for this fairly obvious case.

We work out the event height here, and part of that height is a gap at the bottom, calculated here. It turns out that with recent layout changes, our bar height is 132px but we actually get 127px to render these events.

I wrote a little code to output each row count, and its respective remaining vertical space, with negative space implying an overflow. I then adjusted the bar height down until we didn't get any overflow.


for (var rowCount = 5; rowCount <= 45; rowCount++) { 

  let eventHeight = Math.floor(128 / rowCount); 

  let gap = eventHeight > 8 ? 3 : eventHeight > 6 ? 2 : 1; 

  console.log(rowCount + ':', 127 - (eventHeight * rowCount - gap))  

}

It turns out that compensating by 4 pixels (i.e. assuming the bar height is 128px) was the magic number, so I've updated that.

Oh wild- thank you for the explanation and the update! Looks good.

@breville breville merged commit 6ca5207 into staging Dec 16, 2025
6 checks passed
@breville breville deleted the music-timeline-fix branch December 16, 2025 04:30
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.

3 participants