Skip to content

fix(filmstrip) Excludes partially visible tiles for dominant speaker slot.#16916

Merged
jallamsetty1 merged 1 commit intomasterfrom
fix-filmstrip
Feb 9, 2026
Merged

fix(filmstrip) Excludes partially visible tiles for dominant speaker slot.#16916
jallamsetty1 merged 1 commit intomasterfrom
fix-filmstrip

Conversation

@jallamsetty1
Copy link
Member

No description provided.

Copilot AI review requested due to automatic review settings February 9, 2026 19:51
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@jallamsetty1 jallamsetty1 merged commit c88bfa1 into master Feb 9, 2026
13 of 16 checks passed
@jallamsetty1 jallamsetty1 deleted the fix-filmstrip branch February 9, 2026 22:59
const maxFullyVisible = Math.floor(containerSize / itemSize);

// Fully visible count is the minimum of actual visible and max that can fit fully
// Ensure at least 1 if there are any visible items
Copy link
Member

Choose a reason for hiding this comment

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

@jallamsetty1 This is not quite true!

I would say you either have maxFullyVisible (in this case currentVisibleCount=maxFullyVisible) or maxFullyVisible - 1 (in this case currentVisibleCount=maxFullyVisible+1) full tiles.

It looks like that on some cases you are counting the full tiles with one more (in the second case above).

? _thumbnailWidth + TILE_HORIZONTAL_MARGIN
: _thumbnailHeight + TILE_VERTICAL_MARGIN;
const containerSize = isHorizontal ? _filmstripWidth : _filmstripHeight;

Copy link
Member

Choose a reason for hiding this comment

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

@jallamsetty1 Don't we have very similar logic somewhere?

Maybe we can reuse it?

Copy link
Member

@hristoterezov hristoterezov left a comment

Choose a reason for hiding this comment

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

@jallamsetty1 I'm wondering do we have the same issue on tile view? Should we do something similar? Have you explored this?

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