-
-
Notifications
You must be signed in to change notification settings - Fork 282
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
feat: 5403 - improvements for "price adding" #5404
Changes from 1 commit
a2d8c76
9237415
85fb662
1454d7e
9dc4f8b
f1deed7
c0a0725
5fb06b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,19 +12,19 @@ import 'package:smooth_app/pages/prices/price_product_search_page.dart'; | |
|
||
/// Card that displays the amounts (discounted or not) for price adding. | ||
class PriceAmountCard extends StatefulWidget { | ||
PriceAmountCard({ | ||
const PriceAmountCard({ | ||
required this.priceModel, | ||
required this.index, | ||
required this.refresh, | ||
}) : model = priceModel.priceAmountModels[index], | ||
total = priceModel.priceAmountModels.length; | ||
this.focusNode, | ||
super.key, | ||
}); | ||
|
||
final PriceModel priceModel; | ||
final PriceAmountModel model; | ||
final int index; | ||
final int total; | ||
// TODO(monsieurtanuki): not elegant, the display was not refreshed when removing an item | ||
final VoidCallback refresh; | ||
final FocusNode? focusNode; | ||
|
||
@override | ||
State<PriceAmountCard> createState() => _PriceAmountCardState(); | ||
|
@@ -35,22 +35,33 @@ class _PriceAmountCardState extends State<PriceAmountCard> { | |
final TextEditingController _controllerWithoutDiscount = | ||
TextEditingController(); | ||
|
||
@override | ||
void initState() { | ||
super.initState(); | ||
_controllerPaid.text = _model.paidPrice; | ||
_controllerWithoutDiscount.text = _model.priceWithoutDiscount; | ||
} | ||
|
||
PriceAmountModel get _model => | ||
widget.priceModel.priceAmountModels[widget.index]; | ||
int get _total => widget.priceModel.priceAmountModels.length; | ||
|
||
@override | ||
Widget build(BuildContext context) { | ||
final AppLocalizations appLocalizations = AppLocalizations.of(context); | ||
final bool isEmpty = widget.model.product.barcode.isEmpty; | ||
final bool isEmpty = _model.product.barcode.isEmpty; | ||
return SmoothCard( | ||
child: Column( | ||
children: <Widget>[ | ||
Text( | ||
'${appLocalizations.prices_amount_subtitle}' | ||
'${widget.total == 1 ? '' : ' (${widget.index + 1}/${widget.total})'}', | ||
'${_total == 1 ? '' : ' (${widget.index + 1}/$_total)'}', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not mandatoryuu here, but an improvement would be to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I think you read a bit quickly: here it's not a division (with a decimal separator), it's a lousy "current_item_number/total_number_of_items" description (like "step There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes and no. |
||
), | ||
PriceProductListTile( | ||
product: widget.model.product, | ||
product: _model.product, | ||
trailingIconData: isEmpty | ||
? Icons.edit | ||
: widget.total == 1 | ||
: _total == 1 | ||
? null | ||
: Icons.clear, | ||
onPressed: isEmpty | ||
|
@@ -59,15 +70,18 @@ class _PriceAmountCardState extends State<PriceAmountCard> { | |
await Navigator.of(context).push<PriceMetaProduct>( | ||
MaterialPageRoute<PriceMetaProduct>( | ||
builder: (BuildContext context) => | ||
const PriceProductSearchPage(), | ||
PriceProductSearchPage( | ||
barcodes: widget.priceModel.getBarcodes(), | ||
), | ||
), | ||
); | ||
if (product == null) { | ||
return; | ||
} | ||
setState(() => widget.model.product = product); | ||
_model.product = product; | ||
widget.refresh.call(); | ||
} | ||
: widget.total == 1 | ||
: _total == 1 | ||
? null | ||
: () { | ||
widget.priceModel.priceAmountModels | ||
|
@@ -76,32 +90,32 @@ class _PriceAmountCardState extends State<PriceAmountCard> { | |
}, | ||
), | ||
SmoothLargeButtonWithIcon( | ||
icon: widget.model.promo | ||
? Icons.check_box | ||
: Icons.check_box_outline_blank, | ||
icon: | ||
_model.promo ? Icons.check_box : Icons.check_box_outline_blank, | ||
text: appLocalizations.prices_amount_is_discounted, | ||
onPressed: () => setState( | ||
() => widget.model.promo = !widget.model.promo, | ||
() => _model.promo = !_model.promo, | ||
), | ||
), | ||
const SizedBox(height: SMALL_SPACE), | ||
Row( | ||
children: <Widget>[ | ||
Expanded( | ||
child: PriceAmountField( | ||
focusNode: widget.focusNode, | ||
controller: _controllerPaid, | ||
isPaidPrice: true, | ||
model: widget.model, | ||
model: _model, | ||
), | ||
), | ||
const SizedBox(width: LARGE_SPACE), | ||
Expanded( | ||
child: !widget.model.promo | ||
child: !_model.promo | ||
? Container() | ||
: PriceAmountField( | ||
controller: _controllerWithoutDiscount, | ||
isPaidPrice: false, | ||
model: widget.model, | ||
model: _model, | ||
), | ||
), | ||
], | ||
|
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.
It's risky to change the text this way, as the setter may notify a client calling
setState
… and we're already in the dirty state.To prevent any issue, I would just wrap it around a
addPostFrameCallback
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.
Hmm, I've double-checked and I can't see how this could happen:
_controllerPaid.text
.text
doesn't trigger anything, in particular notonChanged
initState
, when the widget isn't even built, so nosetState
expected.That said, if you prefer a safer way (reviewing-wise) to do exactly the same thing, would you like me to rewrite it like that:
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
TextEditingController
is aValueNotifier
and text is linked thevalue
.Hence the dependency to listeners