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: (Un)select all for history/lists… #4435

Closed
wants to merge 3 commits into from

Conversation

g123k
Copy link
Collaborator

@g123k g123k commented Aug 4, 2023

What

Hi everyone,

Video

SelectAll.mp4

Part of

@g123k g123k requested a review from a team as a code owner August 4, 2023 07:53
@g123k g123k self-assigned this Aug 4, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2023

Codecov Report

Merging #4435 (ff9b568) into develop (cb4a729) will decrease coverage by 0.05%.
Report is 1 commits behind head on develop.
The diff coverage is 2.28%.

@@             Coverage Diff             @@
##           develop    #4435      +/-   ##
===========================================
- Coverage    10.17%   10.13%   -0.05%     
===========================================
  Files          295      295              
  Lines        15099    15199     +100     
===========================================
+ Hits          1537     1540       +3     
- Misses       13562    13659      +97     
Files Changed Coverage Δ
...pp/lib/cards/product_cards/product_title_card.dart 0.00% <0.00%> (ø)
...ric_lib/buttons/smooth_large_button_with_icon.dart 0.00% <0.00%> (ø)
...es/smooth_app/lib/pages/hunger_games/congrats.dart 1.44% <0.00%> (-0.03%) ⬇️
...ooth_app/lib/pages/hunger_games/question_card.dart 0.00% <ø> (ø)
...p/lib/pages/product/common/product_list_modal.dart 0.00% <0.00%> (ø)
...ackages/smooth_app/lib/widgets/smooth_app_bar.dart 49.54% <0.00%> (-2.34%) ⬇️
packages/smooth_app/lib/widgets/smooth_text.dart 0.00% <0.00%> (ø)
...ooth_app/lib/pages/hunger_games/question_page.dart 2.28% <3.14%> (+1.39%) ⬆️

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

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2023

You can test this PR on: Android

@g123k g123k linked an issue Aug 4, 2023 that may be closed by this pull request
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!
The code about select/unselect is good - the "unselect all" icon is not relevant though.
The rest of the PR (80%) is for another issue I guess.

: appLocalizations.compare_products_select_all_items,
icon: Icon(
_allItemsSelected
? Icons.library_add_check_rounded
Copy link
Contributor

Choose a reason for hiding this comment

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

That icon ("check") is not really appropriate, is 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.

It will be a plus, when we want to add all.
And the check is just like on GMail for instance:

Screenshot 2023-08-04 at 13 38 18

Copy link
Contributor

Choose a reason for hiding this comment

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

So in GMail it's "+" when you want to select all items and "check" if you want to UNselect all items, right? Not intuitive.
In hotmail it's "empty rounded check" when not all items are selected and "full rounded check" when all are selected. Makes slightly a little more sense (not that much).
In Android Studio (commit) it's "check", "uncheck" and "undefined", which is more or less what I assumed.

@teolemon
Copy link
Member

teolemon commented Aug 4, 2023

I was thinking of something like a button with text appearing on top of the view

@g123k
Copy link
Collaborator Author

g123k commented Aug 4, 2023

I was thinking of something like a button with text appearing on top of the view

Mmm not sure to understand what you mean, could you create a quick mockup of it please?

@monsieurtanuki
Copy link
Contributor

I was thinking of something like a button with text appearing on top of the view

Mmm not sure to understand what you mean, could you create a quick mockup of it please?

@g123k Perhaps #1392

@teolemon
Copy link
Member

teolemon commented Aug 5, 2023

That's some serious mock-up necromancy @monsieurtanuki 😲

@g123k
Copy link
Collaborator Author

g123k commented Aug 5, 2023

That's some serious mock-up necromancy @monsieurtanuki 😲

Would that UI be better? I can easily change it

@monsieurtanuki
Copy link
Contributor

That's some serious mock-up necromancy @monsieurtanuki 😲

I would say that it looks like something that the cat dragged in! 😸

@teolemon
Copy link
Member

teolemon commented Aug 6, 2023

The mockup is too focused on function.

  • 2 buttons are probably over the top
  • it could be just a clickable centered blue text that is either "Select all" when compare mode is entered or "Unselect all" after a significant amount of items have been selected.
  • I'm not completely clear at the moment.
  • as a reference, here's what GitHub has

Screenshot_20230806-111235.png

Screenshot_20230806-102415.png

@monsieurtanuki
Copy link
Contributor

@teolemon Just to repeat myself, in old fashioned UI what you describe is dealt with with 3 buttons:

  • a "checked" button stating that everything is already selected - if you click you unselect everything
  • an "unchecked" button stating that nothing is selected - if you click you select everything
  • an "undetermined" button stating that some items are selected - if you click you select everything

Cf. "changes" here, as "undetermined":
Capture d’écran 2023-08-06 à 11 17 05

For the record I did not approve this PR only because most of it was unrelated to the "(un)select" feature.
Beyond that my only concern was the choice of icon (not my cup of tea but why not) and perhaps managing the case when only some items are selected ("my" undetermined button).

With text buttons, you have to be very lucky if it fits the screen (at least mine). Or put it in a second line.

@g123k g123k marked this pull request as draft August 6, 2023 15:58
@g123k
Copy link
Collaborator Author

g123k commented Aug 6, 2023

As nobody is OK with the implementation, I am making this PR a draft (the feature freeze will also happen soon).
I have a better idea, but I have to prototype a little bit before.
Don't worry, I will keep you in touch before I begin any development.

@g123k
Copy link
Collaborator Author

g123k commented Aug 15, 2023

Here is another version for this selection mode.
The idea is still to have a checkbox with 3 states, "à la GMail".

But this time, I move the actions to the Bottom App Bar, where all linked actions are located at the same place.
Google Pixel 6 Pro 2

@monsieurtanuki
Copy link
Contributor

@g123k The thing here is that "compare" is not the only possible action - what about "delete" or "share"? Or even the new "dev mode / compare" feature?

How does it work in the "Memo" app on my old samsung (I only quote from the best...)?

not in "select" mode "more" will start "select" mode (so would long press)
Screenshot_2023-08-15-08-31-46 Screenshot_2023-08-15-08-37-33
1 selected item and 3 actions 2 selected items and 3 actions
Screenshot_2023-08-15-08-31-56 Screenshot_2023-08-15-08-32-07

@g123k
Copy link
Collaborator Author

g123k commented Aug 15, 2023

The problem with sub-menus is 0 in terms of discoverability.
My idea was to split the AppBar in two: the top with secondary actions and the bottom with the main => comparison mode

@monsieurtanuki
Copy link
Contributor

The problem with sub-menus is 0 in terms of discoverability.

So are long press, swipe down and swipe left then!

If really you want something more explicit, my suggestions:

  • make the FAB an explicit "multi-select" button, that opens the multi-select mode (so would a long press)
  • when in multi-select mode, show as icons only the main actions (share, compare) and hide the less frequent (delete) behind a "more"

Possible icon for the multi-select FAB:
Capture d’écran 2023-08-15 à 12 56 18

For the record, the long press that opens the multi-select mode (with actions as icons on top) is also there in Evernote, my standard SMS manager and whatsapp. They use the FAB for another purpose.

I mean, it's a standard way to deal with multi-selection.

@g123k
Copy link
Collaborator Author

g123k commented Aug 15, 2023

You're right, that's still not OK, and this long press is indeed a bad/hidden pattern.
In your example, users know they can long press on a list of messages, but clearly not for a list of products, so we should be more explicit about this.

I will iterate a bit more on this, especially as this PR is not necessarily a priority.

@monsieurtanuki
Copy link
Contributor

@g123k For the record I agreed with your initial implementation. I was not convinced by some icons but not a big problem.
The thing was that you (accidentally) put code that was not related at all, that's why I did not approve the PR.

If I can make a suggestion, just push a clean PR that matches your OP video, and thanks to that there will be a new feature: the "(un)select all".

Then, we'll worry about the different actions and how to display them, in different PRs.
First the feature for the users, in not such a bad UI/UX.

@g123k
Copy link
Collaborator Author

g123k commented Aug 25, 2023

I will close this PR as we think about revamping multiple screens, including this one

@g123k g123k closed this Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

It is impossible to make a select all comparison
4 participants