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: #878 - added a multiselect mode to product list page #1035

Merged
merged 35 commits into from
Jan 31, 2022

Conversation

monsieurtanuki
Copy link
Contributor

@monsieurtanuki monsieurtanuki commented Jan 28, 2022

Impacted files:

  • personalized_ranking_page.dart: added a fromItems constructor for more flexibility
  • product_list_item_simple.dart: added optional onTap parameter in order to override the default behavior when checking/unchecking
  • product_list_page.dart: added a "multiselect" mode
  • product_query_page.dart: refactored
  • scan_page_helper.dart: refactored
  • smooth_it_model.dart: for more flexibility, now using List<Product> instead of ProductList
  • smooth_product_card_found.dart: added optional onTap parameter in order to override the default behavior when checking/unchecking

I'm 99% sure that you won't like the UI, but I couldn't think of another UX...

Product list page, with a new multi-select FAB:
Simulator Screen Shot - iPhone 8 Plus - 2022-01-28 at 10 17 22

Multi-select mode, with a ranking FAB that goes to the ranking page with the selected items only:
Simulator Screen Shot - iPhone 8 Plus - 2022-01-28 at 10 18 38

Impacted files:
* `personalized_ranking_page.dart`: added a `fromItems` constructor for more flexibility
* `product_list_item_simple.dart`: added optional `onTap` parameter in order to override the default behavior when checking/unchecking
* `product_list_page.dart`: added a "multiselect" mode
* `product_query_page.dart`: refactored
* `scan_page_helper.dart`: refactored
* `smooth_it_model.dart`: for flexibility, now using `List<Product>` instead of `ProductList`
* `smooth_product_card_found.dart`: added optional `onTap` parameter in order to override the default behavior when checking/unchecking
Impacted files:
* `product_list_page.dart`: added header with buttons; removed FABs
* `ranking_floating_action_button.dart`: added a comment about getting rid of this class because of possible confusion
Impacted files:
* `product_list_page.dart`: added header with buttons; removed FABs
@monsieurtanuki
Copy link
Contributor Author

I've just pushed this:
Simulator Screen Shot - iPhone 8 Plus - 2022-01-28 at 11 47 54

Simulator Screen Shot - iPhone 8 Plus - 2022-01-28 at 11 50 51

It's more like what you had in mind I guess, only less beautiful :)

@jasmeet0817 jasmeet0817 self-requested a review January 28, 2022 13:46
@jasmeet0817
Copy link
Contributor

jasmeet0817 commented Jan 28, 2022

Thank you @monsieurtanuki the code looks great, but question on the UI.

  1. Do we need the AppBar in this page? There's no back button and in the mocks the title "History" appears in the header on the page, on the left of "Compare" button. Although "14/16 products selected" is perhaps useful (but I can't make up my mind on it). But in the mocks the title on the left and compare button on the right definitely looks better than just compare button in the center.
  2. What do the two selector buttons do? I'm guessing they are used for select all or select none (which is great), but why is one of them checked already (when not everything is selected) and can't we live with just one ? (I guess this also ties into (1), if we want to have this checkbox, where will it go without AppBar). @teolemon any comment ?
    image

@teolemon
Copy link
Member

This could be useful info, but:

  • inline instead of the topbar
  • "Select all" - "Select none" instead of the checkboxes (icons are too ambiguous)
  • The number of products could be in the compare button ?

Impacted file:
* `product_list_page.dart`: closer to the mocks
@monsieurtanuki
Copy link
Contributor Author

My point was to reproduce a standard behavior seen on standard apps, as I don't see the added value of reinventing the wheel.
For instance, you seem to be pretty relaxed about using two labels ("History" and "Compare Mode") in buttons on the same line. That mean we should hope that in no language both are long. For the record in French "History" is spelled "Mes produits récemment vus"... And instead of "History", in general the product list name is expected. Better be short.

Anyway, I've just pushed something close to the mocks:

Simulator Screen Shot - iPhone 8 Plus - 2022-01-28 at 17 12 06

Simulator Screen Shot - iPhone 8 Plus - 2022-01-28 at 17 12 31

I'm in favor of more standard (probably old-fashioned) behaviors - just a matter of taste I guess:
Screenshot_2022-01-28-16-34-48

Impacted file:
* `product_query.dart`: unrelated removal of a harmless duplicate
@teolemon
Copy link
Member

@monsieurtanuki perhaps we can try to replace the title by the history icon ?

@monsieurtanuki
Copy link
Contributor Author

@monsieurtanuki perhaps we can try to replace the title by the history icon ?

Yes we can but we'll still have the problem with other product list titles.
I'll send you a screenshot this afternoon.

@teolemon teolemon linked an issue Jan 30, 2022 that may be closed by this pull request
3 tasks
@monsieurtanuki
Copy link
Contributor Author

Simulator Screen Shot - iPhone 8 Plus - 2022-01-30 at 14 26 52

@M123-dev
Copy link
Member

Just tested this PR and I like the feature to choose which products to compare. The code looks good and I don't want to really join the UI/UX discussion but I have a few minor points, nothing which has to be changed immediately:

  • The "Cancel" button switched position after selecting the first product is selected
  • I am also in favour of keeping some standard behavour (I think we already talked about this a while ago @monsieurtanuki) and I was going to long tap on one of the products out of habit to turn on the compare mode, that would be one thing which we could maybe add.
  • We should probably rename "My recently seen products" to "History" as in the mocks since it's not readable

@monsieurtanuki
Copy link
Contributor Author

Thank you @M123-dev for your comments.

  • The "Cancel" button switched position after selecting the first product is selected

Fair enough, I need to fix this.

  • I am also in favour of keeping some standard behavour (I think we already talked about this a while ago @monsieurtanuki) and I was going to long tap on one of the products out of habit to turn on the compare mode, that would be one thing which we could maybe add.

I'm OK with that too.

  • We should probably rename "My recently seen products" to "History" as in the mocks since it's not readable

As evoked previously, this is not really the problem of the "history" title, it's more the problem of the additional text button on the right. We'll have the same problem with all product lists if they have long titles.
That said, I'm not in charge of the translations and I have no idea how to control the impact to all translations after a mere change in one label in English.
But feel free to rename it.

@jasmeet0817
Copy link
Contributor

Thank you @M123-dev for your comments.

  • The "Cancel" button switched position after selecting the first product is selected

Fair enough, I need to fix this.

  • I am also in favour of keeping some standard behavour (I think we already talked about this a while ago @monsieurtanuki) and I was going to long tap on one of the products out of habit to turn on the compare mode, that would be one thing which we could maybe add.

I'm OK with that too.

  • We should probably rename "My recently seen products" to "History" as in the mocks since it's not readable

As evoked previously, this is not really the problem of the "history" title, it's more the problem of the additional text button on the right. We'll have the same problem with all product lists if they have long titles. That said, I'm not in charge of the translations and I have no idea how to control the impact to all translations after a mere change in one label in English. But feel free to rename it.

Yeah translations in such small real estate is always a problem, lets check this code in, and continue to think about how we can handle this.

Agreed on all other points.

More broader concern I have:
We should also think about what really is "History". After scanning a few products I habitually tried to find them in this tab, but only products that I opened appear here, which was not obvious.

Impacted files:
* `app_en.arb`: added a "compare mode" label
* `app_fr.arb`: added a "compare mode" label
* `product_list_item_simple.dart`: added a `onLongPress` parameter
* `product_list_page.dart`: added longPress-to-selection-mode feature; fixed the cancel button position; localized
@monsieurtanuki monsieurtanuki merged commit 44cd470 into openfoodfacts:develop Jan 31, 2022
@monsieurtanuki
Copy link
Contributor Author

Thank you @teolemon @jasmeet0817 @M123-dev for your feedback!

  • The size of the page title that can still be problematic
  • I miss my "select all" / "select none" buttons, but I couldn't find where to put them
  • I fixed the position of the "cancel" button
  • I implemented the long press to selection mode
  • it's localized
  • probably won't look good on dark mode

@monsieurtanuki
Copy link
Contributor Author

We should also think about what really is "History". After scanning a few products I habitually tried to find them in this tab, but only products that I opened appear here, which was not obvious.

@jasmeet0817 it's just the way it worked so far, I'm not saying it's a good idea. Besides, I cannot scan, therefore I'm not very sensitive about that.
But that would definitely make sense to add each scanned product to the history. Please create an issue for that, if relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a compare button on Product History page
4 participants