Repository navigation
Update Material 3 Demo App - #1530
Conversation
darrenaustin
left a comment
darrenaustin
left a comment
There was a problem hiding this comment.
LGTM. Nice work. Looks so much more polished!
Just had a minor concern about using shrinkWrap below, but it is fine for now.
Couple of things I noticed that we might want to consider for future improvements:
- I think it might look better to use a grid for the entire expanded view rather than two independently scrolling lists. That way everything would scroll together and we could potentially let it expand to three columns if the window is wide enough.
- Do we need the gaps between each group block? It looks a little odd to me to have the white showing between the card colors.
These aren't critical, just something to think about going forward.
It really looks nice. Thx!
| ), | ||
| ), | ||
| return ListView( | ||
| shrinkWrap: true, |
There was a problem hiding this comment.
This is probably fine for now, but we should really look into finding a way around using shrinkWrap here as it can lead to performance issues and we don't really want to encourage its use in an example. You might talk this over with @Piinks and see if she has any suggestions here.
There was a problem hiding this comment.
Actually, I'd love to take a look at this first. We are trying to not recommend shrinkwrap unless absolutely necessary and have been removing it from sample code. It is very costly to performance and should only be used if there isn't an option to use slivers. I'll be able to take a look today and see if we can remove this.
|
@QuncCccccc feel free to land when you are happy. Thanks! |
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
Just noting so we can look at this shrinkwrap issue
| @override | ||
| Widget build(BuildContext context) { | ||
| return ListView( | ||
| shrinkWrap: true, |
There was a problem hiding this comment.
It looks like it is being used here too. @QuncCccccc are these used inside of other lists, or are they being placed in Columns? These should probably be SliverLists instead
| trailing: Expanded( | ||
| child: Padding( | ||
| padding: const EdgeInsets.only(bottom: 20), | ||
| child: showLargeSizeLayout |
There was a problem hiding this comment.
For readability, can you wrap these subtrees in methods that return a Widget?
As in
child: showLargeSizeLayout ? _largeLayout() : _smallLayout()
|
I checked this out and removed shrinkwrap, there were 5 cases of it and from what I can tell they weren't all necessary, so just removing it was an easy fix for some. :) See changes in this commit: fb5084f |
8e16f95 to
4168eb3
Compare
|
Not sure why some tests in web_dashboard failed🙂, any suggestions? @domesticmouse
|
Web dashboard needs to be either updated or dropped from master CI. I'll get a PR together for this today. |
Got it. Thanks! |
|
Please pull latest from |
…tay at bottom of rail
6cb038c to
e5569f1
Compare
|
@Piinks PTAL and confirm your review comments have been appropriately incorporated =) |
This PR is to update the Material 3 Demo app in the experimental folder. The layout of the demo app is adaptive based on the screen size and more newly updated components are added.
Screen.Recording.2022-12-15.at.11.35.08.AM.mov
Fixes #1510
Fixes flutter/flutter#112000
Fixes flutter/flutter#111999
Pre-launch Checklist
///).