-
-
Notifications
You must be signed in to change notification settings - Fork 287
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
fix: UI fixed for the Contributors box and made scrollable #1282
Conversation
@@ -14,72 +13,46 @@ class UserContributionView extends StatelessWidget { | |||
Widget build(BuildContext context) { | |||
return Material( | |||
child: SizedBox( | |||
height: MediaQuery.of(context).size.height * 0.9, | |||
child: Stack( | |||
height: MediaQuery.of(context).size.height * 0.8, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you change the ratio here? @abhay1821
height: MediaQuery.of(context).size.height * 0.9, | ||
child: Stack( | ||
height: MediaQuery.of(context).size.height * 0.8, | ||
child: ListView( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A ListView.builder
would be better here in terms of performance
padding: const EdgeInsets.symmetric(horizontal: 20.0), | ||
margin: const EdgeInsets.only(top: 20.0, bottom: 24.0), | ||
child: Text( | ||
AppLocalizations.of(context)!.contribute, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of evoking multiple times AppLocalizations.of(context)!
and Theme.of(context)
, you should use a single variable
), | ||
SmoothListTile( | ||
text: AppLocalizations.of(context)!.contribute_improve_header, | ||
onPressed: () => _contribute(context)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comma here(to format well your code)
), | ||
), | ||
); | ||
}).toList(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add toList(growable: false)
child: Column( | ||
mainAxisSize: MainAxisSize.min, | ||
children: <Widget>[ | ||
_buildTitle(context), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also make the cross as well as the title scrollable, I don't know about you but this doesn't seem right to me.
I'd recommend to leave it as Column and put the ListView/SingleChildScroll only for Dialogs where we need it or if that's not possible just putting the body inside a scrollable widget
Codecov Report
@@ Coverage Diff @@
## develop #1282 +/- ##
==========================================
+ Coverage 9.13% 9.14% +0.01%
==========================================
Files 158 158
Lines 6372 6372
==========================================
+ Hits 582 583 +1
+ Misses 5790 5789 -1
Continue to review full report at Codecov.
|
Hi @M123-dev @g123k , pr.mov |
Heyy yeah sure @abhay1821 create a new one |
What
Screenshot
Fixes bug(s)
Part of
(please be as granular as possible)