Skip to content
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: The compare feature only accepts two existing products #4139

Merged
merged 3 commits into from
Jun 15, 2023

Conversation

g123k
Copy link
Collaborator

@g123k g123k commented Jun 14, 2023

Hi everyone,

Here are my fixes for the Compare feature on the scanner:

  • Ensure to use the correct icons
  • Only accept existing products in the Compare feature
  • When there is only one product, disable the feature and explain to the user what's the problem
  • Improve accessibility for the two buttons

Here are two videos:

Screenshot_1686727186

It will fix both #4127 and #4130

@g123k g123k self-assigned this Jun 14, 2023
@g123k g123k requested a review from a team as a code owner June 14, 2023 07:58
@github-actions github-actions bot added 🌐 l10n 🥫 Product page 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion… User lists labels Jun 14, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #4139 (74554db) into develop (55b4894) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #4139      +/-   ##
===========================================
- Coverage    10.91%   10.89%   -0.02%     
===========================================
  Files          277      277              
  Lines        13536    13552      +16     
===========================================
  Hits          1477     1477              
- Misses       12059    12075      +16     
Impacted Files Coverage Δ
...oth_app/lib/data_models/continuous_scan_model.dart 0.00% <0.00%> (ø)
...ges/smooth_app/lib/helpers/collections_helper.dart 13.04% <0.00%> (-0.60%) ⬇️
...pp/lib/pages/product/common/product_list_page.dart 0.00% <0.00%> (ø)
...ackages/smooth_app/lib/pages/scan/scan_header.dart 2.08% <0.00%> (-0.42%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @g123k!
Looks good, but please have a look at my comments.

@@ -32,9 +31,11 @@ class _ScanHeaderState extends State<ScanHeader> {
),
);

final int validBarcodes = model.getAvailableBarcodes().length;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
final int validBarcodes = model.getAvailableBarcodes().length;
final List<String> barcodes = model.getAvailableBarcodes().toList(growable: false);

Reasons:

  • confusion between "available" and "valid" barcodes
  • you call again model.getAvailableBarcodes() later in the code
  • "validBarcodes" does not sound like a number

If you really want,

final int numberOfBarcodes = barcodes.length;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem with your toList() is that it will force to create a new List in memory.
The Iterable is enough here just to count the number of items.

duration: SmoothAnimationsDuration.brief,
child: ElevatedButton.icon(
style: buttonStyle,
icon: const Icon(Icons.compare_arrows),
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change RankingFloatingActionButton.rankingIconData instead, for consistency: the same icon for the same action.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason I've changed the icon here, is because the RankingFloatingActionButton.rankingIconData is a trophy, and in personal search, this is the right icon to use :/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What would be the best option according to you?
To differentiate the 🏆 and ↔️?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would always use the same icon, as it's the same operation (product list / selected items / ranking page).

g123k and others added 2 commits June 14, 2023 16:34
@teolemon teolemon merged commit 421661c into openfoodfacts:develop Jun 15, 2023
@g123k g123k deleted the compare branch June 15, 2023 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
♿️ accessibility compare mode 🌐 l10n 🥫 Product page 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion… User lists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

We can compare unknown products Inconsistent icons on the home screen
4 participants