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

Broadcast - Use GridView.builder instead of ListView.builder #867

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

julien4215
Copy link
Contributor

No description provided.

lib/src/view/broadcast/broadcast_screen.dart Outdated Show resolved Hide resolved
lib/src/view/broadcast/broadcast_screen.dart Outdated Show resolved Hide resolved
);
child: Padding(
padding: Styles.bodyPadding,
child: GridView.builder(
Copy link
Contributor

Choose a reason for hiding this comment

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

The number of elements is not that large, I wonder if the GridView.count would be more appropriate? The code would be a little easier to read I think.

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 don't know how it would affect performances. According to the GridView.builder documentation the builder is called only for those children that are actually visible. From what I understand the GridView.count will create all widgets at once but it will only render them when they are visible on the screen whereas the GridView.builder will create only the widgets that are needed.

Does the number of elements that is not that large applies to the puzzles too ?

@veloce
Copy link
Contributor

veloce commented Jul 17, 2024

Looks like you based this PR on the websocket branch. You should base it on main branch so I can only review the changes related to the GridView. Thanks.

@julien4215 julien4215 marked this pull request as draft July 17, 2024 11:11
@julien4215
Copy link
Contributor Author

Looks like you based this PR on the websocket branch. You should base it on main branch so I can only review the changes related to the GridView. Thanks.

Ah I thought you would review this after the websocket PR. You can view the changes related only to the GridView by looking only at the last commit but if you want to merge this one before the websocket, I'll rebase it on main.

@julien4215 julien4215 marked this pull request as ready for review July 17, 2024 19:52
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.

None yet

2 participants