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

API to prevent nodes from being selectable #169

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

boriskor
Copy link

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

@jameskerr
Copy link
Member

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

@jameskerr
Copy link
Member

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

@jameskerr
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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.

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

Successfully merging this pull request may close these issues.

Easy api to prevent nodes from being selectable
2 participants