Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 11 commits into from
Closed

feat: Add receipt and price icons to prices screen #6421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
eedb630
feat: Add receipt and price icons to prices screen
vinay769 Mar 6, 2025
db1f5ec
Merge branch 'develop' into feature/receipt-price-icons-ui
vinay769 Mar 8, 2025
9df5045
style: Format code to pass CI/CD formatting check
vinay769 Mar 8, 2025
9804cd2
style: Format code to pass CI/CD formatting check
vinay769 Mar 8, 2025
3caf9ef
style: Format code to pass CI/CD formatting check
vinay769 Mar 8, 2025
b5ef09c
chore: Remove comments, revert pubspec.lock, GeneratedPluginRegistran…
vinay769 Mar 10, 2025
390257b
Merge origin/develop: Integrate latest develop changes
vinay769 Mar 13, 2025
5fa1dc4
updating the price_proof_card
vinay769 Mar 15, 2025
fb61397
feat: Add receipt and price tag icons, support long labels
vinay769 Mar 16, 2025
26fe393
removed packages/smooth_app/pubspec.lock
vinay769 Apr 7, 2025
8706ee5
removed packages/smooth_app/macos/Flutter/GeneratedPluginRegistrant.s…
vinay769 Apr 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
144 changes: 68 additions & 76 deletions packages/smooth_app/lib/pages/prices/price_proof_card.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,30 +74,17 @@ class PriceProofCard extends StatelessWidget {
child: SmoothLargeButtonWithIcon(
text: !model.hasImage
? appLocalizations.prices_proof_find
: model.proofType.getTitle(appLocalizations),
: model.proofType == ProofType.receipt
? appLocalizations.prices_proof_receipt
: appLocalizations.prices_proof_price_tag,
leadingIcon: !model.hasImage
? const Icon(_iconTodo)
: const Icon(_iconDone),
onPressed: model.proof != null
? null
: () async {
final List<_ProofSource> sources =
_ProofSource.getPossibleProofSources(
includeMyProofs: includeMyProofs,
);
// not very likely
if (sources.isEmpty) {
return;
}
final _ProofSource? proofSource;
if (sources.length == 1) {
proofSource = sources.first;
} else {
proofSource = await _ProofSource.select(
context,
sources: sources,
);
}
final _ProofSource? proofSource =
await _ProofSource.select(context);
Comment on lines +86 to +87
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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?

if (proofSource == null) {
return;
}
Expand All @@ -110,26 +97,54 @@ class PriceProofCard extends StatelessWidget {
),
LayoutBuilder(
builder: (BuildContext context, BoxConstraints constraints) => Row(
children: (const <ProofType>[
ProofType.receipt,
ProofType.priceTag,
])
.map<Widget>(
(final ProofType item) => SizedBox(
width: constraints.maxWidth / 2,
child: RadioListTile<ProofType>(
title: Text(item.getTitle(appLocalizations)),
value: item,
children: <Widget>[
Copy link
Contributor

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.

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!,
Comment on lines +108 to +111
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

),
),
)
.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,
),
),
],
Comment on lines +103 to +144
Copy link
Preview

Copilot AI May 16, 2025

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.

),
),
],
Comment on lines +100 to +147
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

),
),
],
Expand All @@ -143,18 +158,6 @@ enum _ProofSource {
gallery,
history;

String getTitle(final AppLocalizations appLocalizations) => switch (this) {
_ProofSource.camera => appLocalizations.settings_app_camera,
_ProofSource.gallery => appLocalizations.gallery_source_label,
_ProofSource.history => appLocalizations.user_search_proofs_title,
};

IconData getIconData() => switch (this) {
_ProofSource.camera => Icons.camera_rounded,
_ProofSource.gallery => Icons.perm_media_rounded,
_ProofSource.history => Icons.document_scanner_rounded,
};

Future<void> process(
final BuildContext context,
final PriceModel model,
Expand Down Expand Up @@ -189,40 +192,29 @@ enum _ProofSource {
}
}

static List<_ProofSource> getPossibleProofSources({
required final bool includeMyProofs,
}) {
final List<_ProofSource> result = <_ProofSource>[];
final bool hasCamera = CameraHelper.hasACamera;
if (hasCamera) {
result.add(_ProofSource.camera);
}
result.add(_ProofSource.gallery);
if (includeMyProofs) {
result.add(_ProofSource.history);
}
return result;
}

static Future<_ProofSource?> select(
final BuildContext context, {
required final List<_ProofSource> sources,
}) async {
static Future<_ProofSource?> select(final BuildContext context) async {
Copy link
Contributor

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.

final AppLocalizations appLocalizations = AppLocalizations.of(context);
final bool hasCamera = CameraHelper.hasACamera;

return showSmoothListOfChoicesModalSheet<_ProofSource>(
context: context,
title: appLocalizations.prices_proof_find,
labels: sources.map<String>(
(_ProofSource source) => source.getTitle(appLocalizations),
),
prefixIcons: sources
.map<Widget>(
(_ProofSource source) => Icon(source.getIconData()),
)
.toList(),
labels: <String>[
if (hasCamera) appLocalizations.settings_app_camera,
appLocalizations.gallery_source_label,
appLocalizations.user_search_proofs_title,
],
prefixIcons: <Widget>[
if (hasCamera) const Icon(Icons.camera_rounded),
const Icon(Icons.perm_media_rounded),
const Icon(Icons.document_scanner_rounded),
],
addEndArrowToItems: true,
values: sources,
values: <_ProofSource>[
_ProofSource.camera,
Copy link
Preview

Copilot AI May 16, 2025

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.

Suggested change
_ProofSource.camera,
if (hasCamera) _ProofSource.camera,

Copilot uses AI. Check for mistakes.

_ProofSource.gallery,
_ProofSource.history,
],
);
}
}
10 changes: 10 additions & 0 deletions packages/smooth_app/lib/pages/prices/proof_type_extensions.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import 'package:flutter/material.dart';
import 'package:flutter_gen/gen_l10n/app_localizations.dart';
import 'package:openfoodfacts/openfoodfacts.dart';

Expand All @@ -11,4 +12,13 @@ extension ProofTypeExtension on ProofType {
// not used for the moment
ProofType.shopImport => 'Shop import',
};

IconData getIcon() => switch (this) {
ProofType.receipt => Icons.receipt_long,
ProofType.priceTag => Icons.local_offer,
// TODO: Handle this case.
ProofType.gdprRequest => throw UnimplementedError(),
// TODO: Handle this case.
ProofType.shopImport => throw UnimplementedError(),
Comment on lines +19 to +22
Copy link
Preview

Copilot AI May 16, 2025

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.

Suggested change
// 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.

};
}
44 changes: 0 additions & 44 deletions packages/smooth_app/macos/Flutter/GeneratedPluginRegistrant.swift
View file
Open in desktop

This file was deleted.

Loading
Loading