learningequality / studio

Content curation tools for Kolibri
https://studio.learningequality.org/
MIT License
110 stars 160 forks source link

Clipboard indeterminate selection incorrect #4636

Closed bjester closed 3 days ago

bjester commented 1 month ago

Observed behavior

When a topic or folder has descendants selected, but itself isn't selected, it shows as unselected, with ancestors selected. image_720

Additionally, there's also a minor cosmetic fix for the alignment of the checkbox, which could be fixed along with this issue.

Expected behavior

A topic/folder should be shown as indeterminately unselected when it has descendants selected, but itself isn't selected. image_720

User-facing consequences

The indeterminate states are meant to make all possible selection states clear to the user. While this is inherently complex in its nature, it's the appropriate manner for visualizing each state, without overlap.

Additional behavior

Replicate the behavior that currently exists in production. As this is likely a regression in the checkbox itself, the state management is unlikely the issue, so attention should be focused towards producing the desired states with the checkbox. Since we migrated to using KDS checkboxes, the issue might be the lack of support for the indeterminately unselected state.

State Description Screenshot
All selected All items within a channel are selected image
Channel partially selected A subset of a channel's items are selected image
Folder unselected A folder itself is unselected, while its descendants are at least partially selected image
Folder partially selected A folder itself is selected but its descendants are partially selected image

Steps to reproduce the issue

  1. Copy a folder to the clipboard
  2. Open the clipboard
  3. Select a subset of the folder's nodes
  4. Observe the folder is shown as indeterminately selected instead of indeterminately unselected

Usage Details

MisRob commented 1 month ago

@bjester

the issue might be the lack of support for the indeterminately unselected state

Checkbox supports indeterminate state https://design-system.learningequality.org/kcheckbox/#states

So most likely a bug either in Studio or in KDS.

bjester commented 1 month ago

@MisRob KCheckbox supports 'indeterminate' but not 'indeterminate unselected' from what I can see. The docs also say, in regards to the prop, it "Overrides checked state." As you can see in the 'Folder unselected' state, this indeterminate unselected state is gray, because the folder itself isn't selected. So this usage must not override the checked state for proper rendering.

Looking at the source, you can see that the only indeterminate state will render with the primary color (notBlank). https://github.com/learningequality/kolibri-design-system/blob/ce9e70750e8fe4067b28923305adfe43f646120b/lib/KCheckbox.vue#L26-L43

MisRob commented 1 month ago

@bjester Okay. I didn't understand what you mean by 'indeterminately unselected'. Yes, this would be a new state for KCheckbox.

MisRob commented 1 month ago

With that I am not suggesting we add a new gray state yet to KDS. Do we know if this is Studio specific - was gray in the designs or was this Vuetify related? I am a bit suspicious this is just a mismatch between the two libraries, and wonder if it'd make sense to have consistent experience and go with the current pattern that is used elsewhere in our products. If there's clarity in that this was intentional rather than just Vuetify's default or there is an intentional nuance I a missing, no problem. I just don't quite yet get what would be the value for the user and if they would be able to understand blue indeterminate selected vs gray indeterminate unselected if they saw them next to each other. For most of the people I'd guess the nature of the indeterminate is simply somewhat..indeterminate :) Alternatively we could change blue to gray for the current indeterminate that would take effect everywhere. Wouldn't like to speculate too much here though, this sounds rather like something for designers to have final call on anyways.

Generally, we will be occasionally running into situations where KDS migration will cause some visual changes to be talked through.

bjester commented 1 month ago

With the current implementation of KCheckbox, there are a total of 3 states for its visual appearance. When selecting items for bulk copy/import or move into a destination, the clipboard selection allows users to specify the conditional selection of a folders along with nodes, which creates more states than the checkbox currently allows. When the user selects a subset of a folder's nodes, and the checkbox becomes indeterminate, then that matches the design expectation where the documentation says: such as when a subset of a topic is selected.

That feature of the clipboard is an intentional UX decision because it allows the users to maintain the folder organization of nodes during the bulk operation. If the user selects a subset of a folder's nodes and the folder itself, then it's indeterminately selected (blue). If the user instead wishes to omit the folder itself from the operation, then it's indeterminately unselected (gray). You can see these in the 'Additional behavior' I outlined in the issue body. The Vuetify library did support this because it's possible from the implementation perspective of the checkbox. I agree users may not immediately know what this means. I don't know that we can provide the functionality we intend, while upholding indeterminate as a subset of a topic is selected, without a fourth state, because with three, the UX is less clear-- 2 states would have to use the same visual appearance.

MisRob commented 1 month ago

Thanks for elaborating @bjester - I think I can understand better and agree it is important to preserve distinguishing them for bulk selections - otherwise the underlying feature of having choice to preserve/not-preserve would not be communicated very clearly. Can't think of anything else either. It sounds like an advanced operation and I guess people using the clipboard frequently will learn the difference over time.

One thing that may help and just a side note at this point, sounds like a good candidate for a nice popup during onboarding tutorial for new users to Studio, shall we ever have that :) Perhaps after it is explored in Kolibri. I imagine it could be handy for more reasons.

MisRob commented 1 month ago

I'm digging into this further to understand very well so that it informs upcoming KDS migration work that I sometimes open issues for. I think that to prevent from regression like this, it'll be always good to gather the most unusual/tricky places in Studio and decide if there is something that needs to be projected to KDS. If yes, then we'd update KDS, and only then proceed to the Studio refactor itself. We probably won't plan for everything that the refactoring process will reveal, but perhaps it will help a bit.

bjester commented 1 month ago

@MisRob I agree, there's very likely more we can do to make this clearer to the user. I think the hierarchical selection structure limits us in what we can change. So unless we decide to change that design, I think enhancing the UI's explanation of itself should add great value.

nucleogenesis commented 3 weeks ago

I think that we might be able to hack the expected visual behavior with some changes to how the isChecked get/set handlers work and applying some conditional styles for the color of the fill on the checkbox.

Basically, KCheckbox will put a check-mark in the box if isChecked=true, even if indeterminate=true.

Seems like we can handle the four states on the Studio side in the short-term if needed, but I suspect that we can do a non-breaking change to KCheckbox.

To be clear on the expectations here, we have the following checkbox states which need to be covered:

Selected Not Selected
Indeterminate [-] (blue) [-] (gray)
Not... [x] (blue) [ ]

I think we can handle this without any breaking changes assuming that we don't inadvertently pass indeterminate as true along with checked together anywhere and are only using that particular pattern here. If Kolibri is using KCheckbox without making indeterminate and checked mutually exclusive.

nucleogenesis commented 3 weeks ago

This will need significant regression testing, but https://github.com/learningequality/kolibri-design-system/pull/744 appears to fix the problem.