-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Added Material Navigation_Drawer in material demo #340
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
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.
Thanks for the contribution, this looks great!
Just a few small pieces of feedback :)
Co-authored-by: Rami <2364772+rami-a@users.noreply.github.com>
Co-authored-by: Rami <2364772+rami-a@users.noreply.github.com>
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 looks really good!
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.
Thanks so much for adding this! Looks good with just one last comment
Thanks @clocksmith |
|
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.
Looking great, sorry just a couple more small suggestions
title: Text( | ||
localization.demoNavigationDrawerToPageOne, | ||
), | ||
trailing: const Icon(Icons.favorite, color: Colors.red), |
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.
Nit: Add these as leading
instead of trailing
and remove the color and let them use the default color.
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.
ok sure
title: Text( | ||
localization.demoNavigationDrawerToPageTwo, | ||
), | ||
trailing: const Icon(Icons.favorite, color: Colors.red), |
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.
Nit: Make this one a different icon, something like Icons.comment
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.
sure
title: Text( | ||
localization.demoNavigationDrawerToPageOne, | ||
), | ||
leading: const Icon(Icons.favorite, color: Colors.red), |
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.
Sorry if this wasn't clear in my earlier comment but please also remove the hardcoded color here too, and then it should be good to submit once @clocksmith approves too.
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.
Sorry i didn't notice it earlier, I'll fix it .
Looks like there is a failing check due to the compiled app size: Could you please update this to 5000 just to be safe: |
Looking into why the golden tests is failing. It seems unrelated to your change @Alabhya268 so please bear with us. cc @perclasson |
Thanks for fixing that @perclasson and thanks again @Alabhya268 for your contribution! |
Thanks @perclasson @rami-a your guidance and advice helped me through my first PR. |
Gallery side - #334
Let me know if any thing needs to be changed