Added trailing button to delete cart item in provider_shopper App #571
Conversation
There was a problem hiding this comment.
Sweet! Thanks.
I have some comments but conceptually LGTM.
In general, we don't want to add more functionality than what is absolutely needed to understand the concept being explained. Code samples are documentation, not apps.
But in this case, your additional code teaches an important lesson: do notifyListeners every time. So, this checks out.
|
|
||
| void remove(Item item) { | ||
| _itemIds.remove(item.id); | ||
| // This line tells [Model] that it should rebuild the widgets that |
There was a problem hiding this comment.
No need to repeat the comment verbatim. The sample code should read like a tutorial of sorts.
Something like:
// Don't forget to tell dependant widgets to rebuild _every time_ you change the model.| icon: Icon(Icons.delete), | ||
| onPressed: () { | ||
| cart.remove(cart.items[index]); | ||
| }), |
There was a problem hiding this comment.
I'd add a trailing comma after } here.
|
@filiph I've made necessary changes please have a look . |
|
|
||
| void remove(Item item) { | ||
| _itemIds.remove(item.id); | ||
| // Don't forget to tell dependant widgets to rebuild _every time_ you change the model. |
There was a problem hiding this comment.
This is over 80 characters, please add a newline.
There was a problem hiding this comment.
+1.
nit: "dependent" is the correct spelling.
|
|
||
| void remove(Item item) { | ||
| _itemIds.remove(item.id); | ||
| // Don't forget to tell dependant widgets to rebuild _every time_ you change the model. |
There was a problem hiding this comment.
+1.
nit: "dependent" is the correct spelling.
| @@ -46,6 +46,12 @@ class _CartList extends StatelessWidget { | |||
| itemCount: cart.items.length, | |||
| itemBuilder: (context, index) => ListTile( | |||
| leading: Icon(Icons.done), | |||
| trailing: IconButton( | |||
| icon: Icon(Icons.delete), | |||
There was a problem hiding this comment.
It would be more correct to use a "remove" icon for the button, rather than a "delete" icon.
Consider this one, for example: https://material.io/resources/icons/?icon=remove_circle_outline&style=baseline
|
@filiph @redbrogdon , I've made the changes as requested ,let me know if anything else needs to be changed also I'm a big fan of "The Boring Flutter Development Show" , thanks for all your videos . They are pretty awesome. 💯 |
|
@filiph if there is nothing that needs to be changed , could you please merge this PR . |
|
Thanks for the nudge, @Alabhya268. Before I can merge, @redbrogdon first needs to approve (our repo is set up so that merging is blocked as long as there are reviewers without LGTM). |
@filiph thanks for letting me know this . |
|
LGTM! |
sample side - Closes #570
Let me know if anything needs to be changed.