-
-
Notifications
You must be signed in to change notification settings - Fork 379
feat: Add receipt and price icons to prices screen #6421
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: Add receipt and price icons to prices screen #6421
Conversation
hello @teolemon Ran dart format command and can you please review thanks |
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.
Hey @vinay769
Left some comments
packages/smooth_app/macos/Flutter/GeneratedPluginRegistrant.swift
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6421 +/- ##
==========================================
- Coverage 9.54% 5.84% -3.71%
==========================================
Files 325 490 +165
Lines 16411 29259 +12848
==========================================
+ Hits 1567 1711 +144
- Misses 14844 27548 +12704 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
done with this thanks to @AshAman999 @teolemon please take a look |
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.
Hi @vinay769!
There are conflicts to be fixed, because of recent changes in the master branch.
You should add your IconData
in ProofTypeExtension
.
I'm very worried about the UI.
You're lucky because your screenshots are in English, with small words.
Would you please try with different longer labels? I suggest "Ticket de caisse" and "Etiquette de prix".
packages/smooth_app/macos/Flutter/GeneratedPluginRegistrant.swift
Outdated
Show resolved
Hide resolved
Okay thanks for you valuable feedback @monsieurtanuki i was working on this and get back to you ASAP with great solution |
hello @monsieurtanuki @teolemon
if both the above answer is yes then the UI is ready to merge |
Hi @vinay769! |
hello @monsieurtanuki Would you kindly review the following code and provide feedback for optimization? |
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.
Hi @vinay769!
Please have a look at my comments.
Would you try with 2 rows of radio buttons, instead of 2 columns? Please post the screenshots.
In case my demand is not explicit enough given the polysemous concepts of rows and columns (especially in flutter), let me draw what I intended.
* Column 1 | * Column 2
and
* Row 1 _______________
* Row 2 _______________
packages/smooth_app/macos/Flutter/GeneratedPluginRegistrant.swift
Outdated
Show resolved
Hide resolved
child: RadioListTile<ProofType>( | ||
title: Text(item.getTitle(appLocalizations)), | ||
value: item, | ||
children: <Widget>[ |
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.
You've rolled back the previous code.
Please stick to the previous code unless you have a good reason. Duplicating code is not a good reason.
final BuildContext context, { | ||
required final List<_ProofSource> sources, | ||
}) async { | ||
static Future<_ProofSource?> select(final BuildContext context) async { |
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 stick to the previous code (especially as you've removed bug fixes).
Besides, _ProofSource
has nothing to do with the current PR, therefore I wouldn't expect reviewing that code.
Unless there's a good reason.
Hello @vinay769, You can use the following layout:
This component will be used elsewhere in the app, so don't hesitate to make it generic. |
Thanks @g123k for this. |
Any update on this? |
Hello @g123k, |
/gemini review |
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.
Code Review
This pull request successfully adds icons to the price page for ProofType.receipt
and ProofType.priceTag
, enhancing visual clarity as intended. The change from RadioListTile
to Radio
widgets facilitates this.
However, there are critical concerns regarding file deletions:
pubspec.lock
Deletion: This file is essential for reproducible builds as it locks dependency versions. Deleting it can lead to inconsistent builds and unexpected behavior. This file must be restored to its state before this PR's changes, or regenerated correctly if dependencies were intentionally updated (though no such changes are evident inpubspec.yaml
in this PR).GeneratedPluginRegistrant.swift
Deletion: This file is typically auto-generated and crucial for macOS plugin registration. Its deletion might break macOS builds or functionality if macOS support is intended. Please clarify if this deletion was intentional and provide context, or restore the file.
Additionally, there are a few areas with potential unintended behavioral changes and opportunities for code improvement, detailed in the specific comments. Due to the critical file deletions and potential behavioral changes, I recommend addressing these issues before merging.
Summary of Findings
- Critical File Deletion:
pubspec.lock
: Thepubspec.lock
file has been deleted. This file is crucial for reproducible builds and should be restored or correctly regenerated. - Critical File Deletion:
GeneratedPluginRegistrant.swift
: TheGeneratedPluginRegistrant.swift
file for macOS has been deleted. This could break macOS builds/functionality and needs clarification or restoration. - Potential Behavioral Change:
forcedProofType
: TheforcedProofType
condition is no longer used to disable theonChanged
callback for proof type radio buttons inprice_proof_card.dart
. This might be an unintended behavioral change ifforcedProofType
was meant to make the selection read-only. (Commented) - Code Duplication in UI: The UI for
ProofType.receipt
andProofType.priceTag
radio options inprice_proof_card.dart
is duplicated. Refactoring into a helper method is recommended. (Commented) - Potential Behavioral Change:
includeMyProofs
: The_ProofSource.select
method inprice_proof_card.dart
now seems to always include the 'history' option, whereas previously this was conditional based onincludeMyProofs
. This could be an intended simplification or an oversight. (Commented) - TODOs in
ProofTypeExtension.getIcon()
: The newgetIcon()
method inproof_type_extensions.dart
hasTODO
comments and throwsUnimplementedError
forProofType.gdprRequest
andProofType.shopImport
. While this might be acceptable if these types are not currently used in this UI context, it's good to plan for their handling if they might appear. (Not commented due to severity settings)
Merge Readiness
This pull request should not be merged in its current state due to the critical deletion of pubspec.lock
and GeneratedPluginRegistrant.swift
. These file changes need immediate attention and likely reversal or justification.
Additionally, there are potential behavioral changes related to forcedProofType
and includeMyProofs
that need clarification to ensure no regressions are introduced. The identified code duplication should also be addressed for better maintainability.
Once these critical and high-priority issues are resolved, the PR will be in a much better state for merging. I am unable to approve pull requests, but I recommend further review after these changes are made.
onChanged: model.proof != null | ||
? null | ||
: (final ProofType? proofType) => | ||
model.proofType = proofType!, |
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 appears the forcedProofType
condition is no longer used to disable the onChanged
callback for the radio buttons. Previously, if forcedProofType != null
, the RadioListTile
was disabled. Now, the Radio
widget's onChanged
is only disabled if model.proof != null
.
Was it intentional to remove the disabling effect of forcedProofType
? If forcedProofType
is meant to make the proof type selection non-editable, this change could introduce a regression.
children: <Widget>[ | ||
SizedBox( | ||
width: constraints.maxWidth / 2, | ||
child: Row( | ||
children: [ | ||
Radio<ProofType>( | ||
value: ProofType.receipt, | ||
groupValue: model.proofType, | ||
onChanged: | ||
model.proof != null || forcedProofType != null | ||
? null | ||
: (final ProofType? proofType) => | ||
model.proofType = proofType!, | ||
onChanged: model.proof != null | ||
? null | ||
: (final ProofType? proofType) => | ||
model.proofType = proofType!, | ||
), | ||
), | ||
) | ||
.toList(), | ||
Icon(ProofType.receipt.getIcon()), | ||
const SizedBox(width: 8), | ||
Expanded( | ||
child: Text( | ||
appLocalizations.prices_proof_receipt, | ||
maxLines: 2, | ||
), | ||
), | ||
], | ||
), | ||
), | ||
SizedBox( | ||
width: constraints.maxWidth / 2, | ||
child: Row( | ||
children: [ | ||
Radio<ProofType>( | ||
value: ProofType.priceTag, | ||
groupValue: model.proofType, | ||
onChanged: model.proof != null | ||
? null | ||
: (final ProofType? proofType) => | ||
model.proofType = proofType!, | ||
), | ||
Icon(ProofType.priceTag.getIcon()), | ||
const SizedBox(width: 8), | ||
Expanded( | ||
child: Text( | ||
appLocalizations.prices_proof_price_tag, | ||
maxLines: 2, | ||
), | ||
), | ||
], | ||
), | ||
), | ||
], |
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.
The UI structure for displaying the ProofType.receipt
radio option (lines 101-123) and the ProofType.priceTag
radio option (lines 124-146) is nearly identical. This introduces code duplication.
Could we consider refactoring this into a reusable widget or a helper method? For example, a method like Widget _buildProofTypeRadioOption(BuildContext context, ProofType proofType, BoxConstraints constraints, PriceModel model)
could encapsulate the common logic, making the code more concise and maintainable.
final _ProofSource? proofSource = | ||
await _ProofSource.select(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.
The logic for selecting a _ProofSource
has been simplified. Previously, _ProofSource.getPossibleProofSources
(which accepted an includeMyProofs
parameter) was used to determine the available sources. This method has been removed (Diff 4), and _ProofSource.select
now seems to always include _ProofSource.history
as an option.
If includeMyProofs
was previously false
in some scenarios, this would mean the 'history' option wasn't available. Is it intended for the 'history' option to always be present now, regardless of the context where includeMyProofs
might have been used?
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.
Pull Request Overview
This PR enhances the prices screen by adding distinct receipt and price-tag icons and refactors the proof-type selection UI.
- Introduces a
getIcon()
extension forProofType
to provide corresponding icons - Replaces
RadioListTile
with a customRadio
+Icon
+Text
layout - Simplifies the proof-source selection modal by removing the
includeMyProofs
parameter
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
packages/smooth_app/macos/Flutter/GeneratedPluginRegistrant.swift | Deleted auto-generated plugin registrant (likely unintended) |
packages/smooth_app/lib/pages/prices/proof_type_extensions.dart | Imported Material and added getIcon() extension for ProofType |
packages/smooth_app/lib/pages/prices/price_proof_card.dart | Updated UI to use Radio widgets with icons and simplified selection logic |
Comments suppressed due to low confidence (2)
packages/smooth_app/macos/Flutter/GeneratedPluginRegistrant.swift:1
- Removing the entire GeneratedPluginRegistrant.swift may break macOS plugin registration, as this file is auto-generated and should not be manually deleted.
-//
packages/smooth_app/lib/pages/prices/price_proof_card.dart:87
- The selection now always shows all proof sources and ignores the original
includeMyProofs
flag, altering intended behavior whenincludeMyProofs
is false.
final _ProofSource? proofSource = await _ProofSource.select(context);
// TODO: Handle this case. | ||
ProofType.gdprRequest => throw UnimplementedError(), | ||
// TODO: Handle this case. | ||
ProofType.shopImport => throw UnimplementedError(), |
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.
[nitpick] The getIcon()
method throws UnimplementedError
for gdprRequest
and shopImport
, potentially causing runtime crashes. Consider handling or documenting these cases.
// TODO: Handle this case. | |
ProofType.gdprRequest => throw UnimplementedError(), | |
// TODO: Handle this case. | |
ProofType.shopImport => throw UnimplementedError(), | |
ProofType.gdprRequest => Icons.privacy_tip, // Placeholder icon for GDPR Request | |
ProofType.shopImport => Icons.store, // Placeholder icon for Shop Import |
Copilot uses AI. Check for mistakes.
addEndArrowToItems: true, | ||
values: sources, | ||
values: <_ProofSource>[ | ||
_ProofSource.camera, |
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.
The values
list always includes camera
, but the labels
and prefixIcons
lists conditionally omit it when hasCamera
is false, causing misalignment between labels, icons, and values.
_ProofSource.camera, | |
if (hasCamera) _ProofSource.camera, |
Copilot uses AI. Check for mistakes.
child: Row( | ||
children: [ | ||
Radio<ProofType>( | ||
value: ProofType.receipt, | ||
groupValue: model.proofType, | ||
onChanged: | ||
model.proof != null || forcedProofType != null | ||
? null | ||
: (final ProofType? proofType) => | ||
model.proofType = proofType!, | ||
onChanged: model.proof != null | ||
? null | ||
: (final ProofType? proofType) => | ||
model.proofType = proofType!, | ||
), | ||
), | ||
) | ||
.toList(), | ||
Icon(ProofType.receipt.getIcon()), | ||
const SizedBox(width: 8), | ||
Expanded( | ||
child: Text( | ||
appLocalizations.prices_proof_receipt, | ||
maxLines: 2, | ||
), | ||
), | ||
], | ||
), | ||
), | ||
SizedBox( | ||
width: constraints.maxWidth / 2, | ||
child: Row( | ||
children: [ | ||
Radio<ProofType>( | ||
value: ProofType.priceTag, | ||
groupValue: model.proofType, | ||
onChanged: model.proof != null | ||
? null | ||
: (final ProofType? proofType) => | ||
model.proofType = proofType!, | ||
), | ||
Icon(ProofType.priceTag.getIcon()), | ||
const SizedBox(width: 8), | ||
Expanded( | ||
child: Text( | ||
appLocalizations.prices_proof_price_tag, | ||
maxLines: 2, | ||
), | ||
), | ||
], |
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.
[nitpick] The layout for Receipt
and PriceTag
is duplicated. Consider abstracting common logic (e.g., iterating over a list of types) to reduce repetition and ease future updates.
Copilot uses AI. Check for mistakes.
There is no activity on this PR. Thanks for this contribution @vinay769 |
What
feat
, for Features enhancing User Experience and User InterfaceScreenshot
Part of