dgreene1 / react-accessible-treeview

A react component that implements the treeview pattern as described by the WAI-ARIA Authoring Practices.
https://dgreene1.github.io/react-accessible-treeview
MIT License
277 stars 37 forks source link

:sparkles: Feat: add `allowKeyboardControl` prop to TreeView component #188

Closed angelozdev closed 4 months ago

angelozdev commented 4 months ago

Description of Changes

Added allowKeyboardControl Property

Summary: Introduced a new optional boolean property allowKeyboardControl to the TreeView component. This property allows developers to enable or disable all keyboard interactions within the TreeView. By default, this property is set to true, allowing keyboard interactions.

Changes in Detail:

  1. Interface Changes:

    • Modified ITreeViewProps interface in src/TreeView/index.tsx to include allowKeyboardControl with a default value of true.
  2. Component Logic Updates:

    • Integrated the allowKeyboardControl property into the TreeView component's props.
    • Updated the handleKeyDown function to respect the allowKeyboardControl property. If allowKeyboardControl is set to false, the function will return immediately, preventing any keyboard events from being handled.

Benefits: This change enhances the flexibility of the TreeView component, allowing developers to disable keyboard interactions when necessary, providing better control over the component's behavior in different use cases.

dgreene1 commented 4 months ago

@mellis481 can you check this one out instead?

@angelozdev please make sure you have unit tests. And since I’m not familiar right now, is this adding functionality? Or allowing existing functionality to be turned off?

Because if it’s new functionality, I can’t gaurantee that we’ll be wanting to support additional scope.

angelozdev commented 4 months ago

Hey @dgreene1

These changes mainly let us turn off existing functionality instead of adding anything new. The allowKeyboardControl property lets us disable all keyboard interactions in the TreeView component. So, it's just giving us more control over the current behavior.

I'll add unit tests for this. Sure!

mellis481 commented 4 months ago

These changes mainly let us turn off existing functionality instead of adding anything new. The allowKeyboardControl property lets us disable all keyboard interactions in the TreeView component. So, it's just giving us more control over the current behavior.

@angelozdev Disabling all ability for a user to interact with a tree via their keyboard (even as opt-in functionality) would cause this library to be inaccessible and is not something we will do.

angelozdev commented 4 months ago

@mellis481 I get your concern about accessibility. However, there are certain situations where it makes sense to disable keyboard interactions. For example, when adding functionality to rename a file or folder, having keyboard interactions enabled can interfere with the renaming process. In these cases, turning off keyboard control temporarily can make things smoother for users.

Please reconsider this change.

mellis481 commented 4 months ago

For example, when adding functionality to rename a file or folder, having keyboard interactions enabled can interfere with the renaming process.

@angelozdev Can you please describe this situation where having keyboard functionality enabled causes issues, either with detailed steps or (ideally) screenshots? Because I'm not understanding how a problem could be caused.

angelozdev commented 3 months ago

Hi @mellis481,

Certainly. When a developer wants to implement a feature like renaming, all accessible keyboard interactions can become cumbersome because they don't allow the user to navigate through or edit the new name effectively.

In this specific case, it is necessary and imperative to disable all keyboard interactions until the file or folder renaming is completed.

If this approach isn't suitable, how would you propose we implement this feature?

pr

original

dgreene1 commented 3 months ago

@angelozdev I would recommend that clicking the rename button opens up a dialogue (let’s say a modal) that asks for you to rename it. By doing this, you have a way to accept text input independently of the tree component.

It’s out of scope to have edible nodes on our tree component. FYI, I’m the architect for @mellis481’s team.

angelozdev commented 3 months ago

Hi, @dgreene1

I appreciate your focus on accessibility. However, using a modal for renaming isn't viable as it negatively impacts user experience by adding unnecessary steps. In product development, we must always prioritize user experience over technical aspects. I'm sure we can agree on that.

Temporarily disabling keyboard interactions during renaming ensures a smooth workflow without interruptions. This is the most direct and effective solution to avoid conflicts with keyboard shortcuts.

Can we move forward with this option, or do you have an alternative that maintains user experience without compromises?

dgreene1 commented 3 months ago

Unfortunately user experience is subjective, and the pattern that I recommended (ie the modal) was recommended to us from our company’s User Experience Designers after they conducted research on many real world users/customers.