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.

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

feat: Clearer 'Packaging components' preview and modified the maximum size of the autocomplete tab. #3744

Merged
merged 10 commits into from
Mar 2, 2023
+26 −28
Merged

feat: Clearer 'Packaging components' preview and modified the maximum size of the autocomplete tab. #3744

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d8aed2f
Updated the preview of the package in the 'Packaging components ' tab.
thomas-algo Feb 27, 2023
43cdaab
Autocomplete now gives hints before anything is typed & maxOptionsHei…
thomas-algo Feb 27, 2023
234b88a
Merge branch 'develop' into edit_packaging
thomas-algo Feb 27, 2023
3ee60ab
Removed Autocomplete now gives hints before anything is typed.
thomas-algo Feb 28, 2023
34a0796
Merge branch 'develop' into edit_packaging
monsieurtanuki Feb 28, 2023
4cbea5f
Comment explaining the limit of suggestions.
thomas-algo Feb 28, 2023
e8081a0
Taking into account the case 'Material empty' and 'Weight not empty' …
thomas-algo Feb 28, 2023
4b7b140
Removing unused variables
thomas-algo Feb 28, 2023
36e226b
Merge branch 'develop' into edit_packaging
monsieurtanuki Mar 2, 2023
ee90a53
Update packages/smooth_app/lib/pages/product/edit_new_packagings_help…
monsieurtanuki Mar 2, 2023
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
2 changes: 1 addition & 1 deletion packages/smooth_app/lib/pages/product/edit_new_packagings.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ class _EditNewPackagingsState extends State<EditNewPackagings> {
SmoothCard(
color: _getSmoothCardColorAlternate(context, index),
child: EditNewPackagingsComponent(
title: appLocalizations.edit_packagings_element_title(index + 1),
title: _helpers[index].getSubTitle(),
deleteCallback: () =>
setState(() => _removePackagingAt(deleteIndex)),
helper: _helpers[index],
Expand Down
25 changes: 19 additions & 6 deletions packages/smooth_app/lib/pages/product/edit_new_packagings_helper.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,23 +75,36 @@ class EditNewPackagingsHelper {

void addIfNotEmpty(final String text) {
if (text.isNotEmpty) {
result.add(text);
result.add('$text ');
}
}

addIfNotEmpty(controllerUnits.text);

if (controllerUnits.text.isNotEmpty) {
result.add('${controllerUnits.text} x ');
}

addIfNotEmpty(controllerShape.text);
addIfNotEmpty(controllerMaterial.text);
addIfNotEmpty(controllerRecycling.text);
addIfNotEmpty(controllerWeight.text);
addIfNotEmpty(controllerQuantity.text);

if (controllerMaterial.text.isNotEmpty & controllerWeight.text.isNotEmpty) {
result.add('(${controllerMaterial.text} : ${controllerWeight.text}g)');
}

else if (controllerMaterial.text.isNotEmpty ) { // Therefore controllerWeight.text is empty
result.add('(${controllerMaterial.text})');
}

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
if (controllerMaterial.text.isNotEmpty & controllerWeight.text.isNotEmpty) {
result.add('(${controllerMaterial.text} : ${controllerWeight.text}g)');
}
else if (controllerMaterial.text.isNotEmpty ) { // Therefore controllerWeight.text is empty
result.add('(${controllerMaterial.text})');
}
if (controllerMaterial.text.isNotEmpty) {
if (controllerWeight.text.isNotEmpty) {
result.add('(${controllerMaterial.text} : ${controllerWeight.text}g)');
} else {
result.add('(${controllerMaterial.text})');
}
}

So you don't display the weight if the material is not specified, which is a bit strange.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this is the same behavior as in the final product page. However, it might be interesting to have the mass in both cases (Nevertheless, the case where 'material' is empty and 'weight' is filled is quit rare).

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, I don't know where the changed display was suggested. And how rare the "the case where 'material' is empty and 'weight' is filled".
I would suggest at least to consider this very rare case with an additional test: "if weight is not empty and material is empty, let's display only weight".
By the way in English there's no space before a :. In French it's an unbreakable space.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
if (controllerMaterial.text.isNotEmpty & controllerWeight.text.isNotEmpty) {
result.add('(${controllerMaterial.text} : ${controllerWeight.text}g)');
}
else if (controllerMaterial.text.isNotEmpty ) { // Therefore controllerWeight.text is empty
result.add('(${controllerMaterial.text})');
}
if (controllerMaterial.text.isNotEmpty & controllerWeight.text.isNotEmpty) {
result.add('(${controllerMaterial.text}: ${controllerWeight.text}g)');
}
if (controllerMaterial.text.isNotEmpty & controllerWeight.text.isEmpty) {
result.add('(${controllerMaterial.text})');
}
if (controllerMaterial.text.isEmpty & controllerWeight.text.isNotEmpty) {
result.add('(${controllerWeight.text}g)');
}
// And nothing is added if they are both empty.

Thanks for your advises, here is a version taking this case into account. I am not sure the display change was suggested elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

By the way in English there's no space before a :. In French it's an unbreakable space.

@monsieurtanuki: on the server we have a translatable string 'sep' which is an empty string in English, and a space in French and other languages. Is this something already handled in some way in the app? I didn't see something equivalent in the lib/l10n/app_en.arb file

Copy link
Contributor

Choose a reason for hiding this comment

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

@stephanegigandet We don't have this sep in Smoothie.
Could you list all languages that do have a space, e.g. taken from the server?

Copy link
Contributor

Choose a reason for hiding this comment

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

@monsieurtanuki I just checked, we only do it for French.

=head2 separator_before_colon( $l )

=head3 Arguments

=head4 language code $l

In some languages like French, colons have a space before them.
e.g. "Valeur : 500" in French, "Value: 500" in English

This function returns a non-breaking space character for those languages.

=cut

sub separator_before_colon ($l) {

	if ($l eq 'fr') {
		return "\N{U+00A0}";
	}
	else {
		return '';
	}
}

Maybe we can put a similar function in Smoothie, it's likely to be useful in other places.

if (result.isEmpty) {
return null;
}
return result.join(' ');
return result.join('');
thomas-algo marked this conversation as resolved.
Show resolved Hide resolved
}

/// Returns the packaging subtitle from the controllers
String getSubTitle() {
return controllerRecycling.text;
Copy link
Contributor

Choose a reason for hiding this comment

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

Says who?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved the packaging recycling info to the subtitle of the widget for readability (cf first screenshot). This function generate this subtitle.

}
/// Returns the packaging from the controllers.
ProductPackaging getPackaging() {
final ProductPackaging packaging = ProductPackaging();
Expand Down
18 changes: 4 additions & 14 deletions packages/smooth_app/lib/pages/product/simple_input_text_field.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,16 +52,16 @@ class SimpleInputTextField extends StatelessWidget {
if (input.length < minLengthForSuggestions) {
return <String>[];
}

thomas-algo marked this conversation as resolved.
Show resolved Hide resolved
return OpenFoodAPIClient.getSuggestions(
tagType!,
language: ProductQuery.getLanguage()!,
country: ProductQuery.getCountry(),
categories: categories,
shape: shapeProvider?.call(),
user: ProductQuery.getUser(),
limit: 1000000, // lower max count on the server anyway
input: value.text.trim(),
limit: 15,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 15?

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 limit is 400 on the server. The autocomplete tab doesn't need to be that long. Moreover, if the maximum API limit on the server was increased, nothing would limit the number of items in the suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Therefore your answer is: "15 is the best compromise according to me". Please add a comment about it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I believe that it is the best compromise, it is unnecessary to have more that 15 suggestions as approximately 5 can be seen at once on screen without scrolling.

Copy link
Member

Choose a reason for hiding this comment

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

15 sounds reasonable, but yes a comment would be good 👍🏼

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

input: input,
);
},
fieldViewBuilder: (BuildContext context,
Expand Down Expand Up @@ -109,17 +109,7 @@ class SimpleInputTextField extends StatelessWidget {
options: options,
// Width = Row width - horizontal padding
maxOptionsWidth: constraints.maxWidth - (LARGE_SPACE * 2),
maxOptionsHeight: screenHeight -
(keyboardHeight == 0
? kBottomNavigationBarHeight
: keyboardHeight) -
widgetPosition -
// Vertical padding
(LARGE_SPACE * 2) -
// Height of the TextField
(DefaultTextStyle.of(context).style.fontSize ?? 0) -
// Elevation
4.0,
maxOptionsHeight: screenHeight/3,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just hoping it works.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can see the result on the third screenshot

);
},
),
Expand Down