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

Abilities to set focus by passing focusedId props #180

Merged
merged 6 commits into from
Apr 26, 2024

Conversation

yhy-1
Copy link
Collaborator

@yhy-1 yhy-1 commented Apr 24, 2024

fix the issue in #156

  • If focus is set on the collapsed node, expand the parents
  • Keyboard accessibility working as before

@yhy-1 yhy-1 changed the title Abilies 98888888888888888888888888888888888888888888888888888888888yhuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuu===\=\+ Abilities to set focus by passing focusedId props Apr 24, 2024
@Ke1sy
Copy link
Contributor

Ke1sy commented Apr 25, 2024

One of the possible difficulties for user to use this new prop could be that the ids of the nodes are not displayed in html even if they are provided by user for each node, so without using the hardcoded id for each node user may not even know how they are calculated inside treeview.

@Ke1sy
Copy link
Contributor

Ke1sy commented Apr 25, 2024

I'm not sure, if we need to add any error handling when the focused id node doesn't exist, it looks like it can break the application... But on the other hand this should be handled by the application's ErrorBoundary.
image
image

@Ke1sy
Copy link
Contributor

Ke1sy commented Apr 25, 2024

It looks like the actual focus doesn't move to the focused node, only visual styles appear on new focused node, and in the result the keyboard navigation doesn't work from the new focused place.
image

@Ke1sy
Copy link
Contributor

Ke1sy commented Apr 25, 2024

I noticed that in basic treeview the disabled nodes are focusable with mouse and keyboard, may be can think if we really need this AC: "- shouldn't be able to set focus on a disabled node"
image
CC: @mellis481

website/docs/api.md Outdated Show resolved Hide resolved
website/docs/examples/ControlledFocusedNode/index.js Outdated Show resolved Hide resolved
@mellis481
Copy link
Collaborator

I noticed that in basic treeview the disabled nodes are focusable with mouse and keyboard, may be can think if we really need this AC: "- shouldn't be able to set focus on a disabled node")

I agree that disabled nodes shouldn't be focusable. That said, I think we need to keep them focusable b/c of how the tree was originally developed which allows for children of disabled nodes to NOT be disabled (which doesn't make sense). I detailed this issue here: #93.

@yhy-1
Copy link
Collaborator Author

yhy-1 commented Apr 25, 2024

@Ke1sy @mellis481
I add the check to see if the existing focused ID exists and throw that error in case they set a bad focused ID. If we are saying we should skip the check and just focus if it finds the ID, I can remove it.

@yhy-1
Copy link
Collaborator Author

yhy-1 commented Apr 25, 2024

One of the possible difficulties for user to use this new prop could be that the ids of the nodes are not displayed in html even if they are provided by user for each node, so without using the hardcoded id for each node user may not even know how they are calculated inside treeview.

Hello, I find this statement a bit unclear. If the IDs for the tree aren't hard-coded or if we don't know which ID to pass, how can we determine what the user is trying to focus on? The user already has the ability to set an ID. Is this the issue you're referring to? If so, I can update the example to clarify this point. @Ke1sy

@mellis481
Copy link
Collaborator

@Ke1sy @mellis481 I add the check to see if the existing focused ID exists and throw that error in case they set a bad focused ID. If we are saying we should skip the check and just focus if it finds the ID, I can remove it.

I would recommend we don't throw an error if they pass a bad ID, but rather just don't focus anything.

@mellis481
Copy link
Collaborator

mellis481 commented Apr 25, 2024

One of the possible difficulties for user to use this new prop could be that the ids of the nodes are not displayed in html even if they are provided by user for each node, so without using the hardcoded id for each node user may not even know how they are calculated inside treeview.

Hello, I find this statement a bit unclear. If the IDs for the tree aren't hard-coded or if we don't know which ID to pass, how can we determine what the user is trying to focus on? The user already has the ability to set an ID. Is this the issue you're referring to? If so, I can update the example to clarify this point. @Ke1sy

@Ke1sy It seems to me you are conflating how the developer will code programmatically set focus to a node and the user being able to set focus. Perhaps this has happened b/c you are focused on the Storybook example where we let the user set the value? In any case, the user won't be setting the node to focus.

CC: @yhy-1

if (!focusedId) {
dispatch({
type: treeTypes.clearFocus,
id: treeParentNode.children[0],
Copy link
Collaborator Author

@yhy-1 yhy-1 Apr 25, 2024

Choose a reason for hiding this comment

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

Clear focus. This result in the default uncontrolled tree, ie focus on first element when they tab on tree.

@mellis481 mellis481 merged commit 5aeca46 into master Apr 26, 2024
1 check passed
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.

3 participants