Use BroadcastPlayerWidget where possible#2602
Conversation
There was a problem hiding this comment.
Pull request overview
Expands usage of BroadcastPlayerWidget across broadcast-related UI and updates it so federation flags and player titles can scale with the surrounding text size.
Changes:
- Replaced duplicated “title + name” row rendering with
BroadcastPlayerWidgetin the players list and game result tiles. - Updated
BroadcastPlayerWidgetto size the federation flag and title text based onfontSize. - Minor style refactors (e.g., enum dot-shorthand for
ellipsis).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/src/view/broadcast/broadcast_players_tab.dart | Uses BroadcastPlayerWidget in BroadcastPlayerRow instead of hand-built title/name UI. |
| lib/src/view/broadcast/broadcast_player_widget.dart | Scales federation flag and title with font size; adjusts text overflow/style usage. |
| lib/src/view/broadcast/broadcast_player_results_screen.dart | Uses BroadcastPlayerWidget for opponent title rendering in game result tiles. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Expanded( | ||
| child: BroadcastPlayerWidget(player: player, showRating: false, showFederation: false), | ||
| ), |
There was a problem hiding this comment.
BroadcastPlayerRow now renders the name/title via BroadcastPlayerWidget, but the earlier destructuring still binds title and name (and they are no longer used in this widget). With the repo’s strict linting this can surface as an unused-local warning/failure. Prefer destructuring only the fields you actually use here (e.g. keep federation/rating for the subtitle) or rename unused bindings to _.
| Flexible(child: Text(name ?? '', overflow: TextOverflow.ellipsis)), | ||
| ], | ||
| ), | ||
| title: BroadcastPlayerWidget(player: opponent, showFederation: false, showRating: false), |
There was a problem hiding this comment.
_GameResultListTile no longer uses the locally destructured title/name/rating after switching the title: to BroadcastPlayerWidget, so the destructuring final BroadcastPlayer(:federation, :title, :name, :rating) = opponent; is now partially unused. Please destructure only the needed fields (e.g. federation) to avoid unused-local warnings under the project’s strict analyzer settings.
| Image.asset( | ||
| 'assets/images/fide-fed/$federation.png', | ||
| height: (textStyle?.fontSize ?? 14) - 2, | ||
| ), |
There was a problem hiding this comment.
The federation image height is currently derived from textStyle?.fontSize ?? 14, which ignores the ambient DefaultTextStyle when textStyle is null. This means the flag won’t actually scale with surrounding text size in call sites that rely on DefaultTextStyle.merge (e.g. broadcast_boards_tab.dart). Consider deriving the font size from the effective text style (textStyle ?? DefaultTextStyle.of(context).style) and using that for the image height.
There was a problem hiding this comment.
Looks like a good comment @HaonRekcef
Have you tested how it renders when there is no textStyle?
There was a problem hiding this comment.
Oh my bad you actually added that.
2 participants
Use
BroadcastPlayerWidgetin more places and scale the federation image and the title with thefontSize.