API to prevent nodes from being selectable

[boriskor]

Added a tree prop disableSelect, in the spirit of disableEdit and disableDrag, closes #56

[jameskerr]

Thank you @boriskor ! This looks really good. I am testing it out now.

[jameskerr]

I am noticing some bugs when using shift + up/down to expand the selection with the keyboard.

[jameskerr]

It looks like you may need to keep the the anchor and the most recent the same, even if some of the items are not selected.

Maybe this means you could make a function called tree.filterSelected(nodes/ids) that would take an array of ids or nodes and filter out only the ones that are selectable. Just like you have in the selectAll function. Then just use that function before setting the selection anywhere.

This would allow you to remove the if statements. In that function, if there is no disableSelection prop, just return everything. If there is, run the function on it.

Then make sure to test the shift+up, shift+down behavior and see if it feels right.

Add these lines to the bottom of the gmail-spec.cy.ts file as well:

  it("can select inbox but not categories", () => {
    cy.get("@item").contains("Inbox").click();
    cy.focused().should("have.attr", "aria-selected", "true");
    cy.get("@item").contains("Categories").click();
    cy.focused().should("have.attr", "aria-selected", "false");
  });

  it("select all does not select categories or spam", () => {
    cy.get("@item").contains("Inbox").click();
    cy.focused().type("{meta}a");
    cy.get("[aria-selected='true']")
      .should("not.contain.text", "Categories")
      .should("not.contain.text", "Spam")
      .should("contain.text", "Inbox")
      .should("have.length", 15);
  });

[boriskor]

Hi, @jameskerr, thank you for a quick response and sorry for the delay!

I've added the tests you've suggested to Cypress.

Can you explain please what are the bugs you noticed with shift + up/down and how can I reproduce them, step by step? Not sure I understand. Do you mean that shift + up/down should "skip" non-selectable nodes and move the focus right to the next selectable node? Or something else?

[jameskerr]

Here are some videos of the behavior I am seeing on your branch.

Shift+Up/Down with selectable nodes (base behavior)

CleanShot.2023-08-28.at.10.12.33.mp4

Shift+Up/Down Encountering a non-selectable node (bug)

CleanShot.2023-08-28.at.10.15.01.mp4

To me, it's strange that the first time pressing shift+down, it appears nothing happens because it tries to select "spam", but fails because it's not selectable. Shift+down again selects "Important". Then shift+up does not deselect "Important" which I think it should.

What I think should happen:

When changing the selection with "shift+up/down", pretend like the non-selectable nodes don't exist. If you encounter one as the next node, skip it and select the next selectable node.

In the video above starting with the node "Draft" selected...

1. Pressing shift+down should skip "spam" (not-selectable) and select "Important"
2. Pressing shift+up should then deselect "important" and move the focus back to "drafts"

Implementation

Multi-selection relies on the concept of an "anchor node" and a "most-recently selected node". When you expand the selection with the "shift+up/down" keys, the anchor node stays the same and the most recently selected changes. The new selection ends up being all the nodes between the anchor and the most recent node.

It appears that we need to account for non-selectable nodes when expanding/collapsing the selection. We need to add a check to "select next node" that skips non-selectable nodes and jumps to the next one we can select.

Did I write the words "selectable" and "nodes" enough times? 😂 Hopefully that makes sense.

[enure]

Since this is not yet merged, I'll add that this requested feature can be partially achieved by providing a custom component to the renderRow prop and then conditionally adding a class to the row that disables pointer events based on the node data for that row. You can reference the DefaultRow component in this repo and adjust as needed to add the className. From what I can tell this will at least block selection from tap or click.