Skip to content

Conversation

isuscbrmid
Copy link
Contributor

What does it do?

Currently if I want to hide a node, I can do this with css, but the keyboard feature is able to reach those nodes. We cannot use the "hide" prop in each node, since it is overwritten when "restoringDefaults".

Fixes # (issue)

Add a custom/optional prop to the nodes to disable keyboard navigation on them.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • Updated documentation (if applicable)
  • Added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • My changes generate no new warnings

@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit 62d60f1 and detected 0 issues on this pull request.

View more on Code Climate.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1380

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 93.995%

Totals Coverage Status
Change from base Build 1374: 0.01%
Covered Lines: 599
Relevant Lines: 620

💛 - Coveralls

@mrchief
Copy link
Collaborator

mrchief commented May 25, 2020

I don't think we need a new prop for this. Skipping all keyboard actions on hidden nodes can be the default behavior.

@ellinge Any apparent concerns if we do this?

@mrchief
Copy link
Collaborator

mrchief commented May 25, 2020

Also, please add tests for these changes.

@isuscbrmid
Copy link
Contributor Author

Also, please add tests for these changes.

no sure if I understand, more tests using that prop?

@isuscbrmid
Copy link
Contributor Author

I don't think we need a new prop for this. Skipping all keyboard actions on hidden nodes can be the default behavior.

@ellinge Any apparent concerns if we do this?

we need that because on

we set the wholes nodes to false. So "hidden prop" will not work

@MJRuskin
Copy link
Contributor

@isuscbrmid would you be ok with something like this #371 ?

Any nodes that were initially hidden in the data will remain hidden (and skipped by the keyboard navigation).
They only show up during search.

Unless of course you want a completely hidden "fake" node, and then you can label it "" which makes it is also unsearchable.

@mrchief
Copy link
Collaborator

mrchief commented May 26, 2020

We cannot use the "hide" prop in each node, since it is overwritten when "restoringDefaults".

Not sure I understand this bit.

In general, I'm usually hesitant to add more props like these ones as it increases the maintenance area in general while it provides little value to only a handful of edge cases.

You also have to analyse the impact of this change from an accessibility standpoint.

Also, can we have a CodeSandbox to reproduce the problem?

@isuscbrmid
Copy link
Contributor Author

isuscbrmid commented May 26, 2020 via email

@isuscbrmid
Copy link
Contributor Author

#371

yes it seems it can be the solution, I will double check if this one work, will close my Pr

@mrchief
Copy link
Collaborator

mrchief commented May 26, 2020

We cannot use the "hide" prop in each node, since it is overwritten when "restoringDefaults".

I guess I'm confused because there is no such prop; or a method called restoringDefaults.

@isuscbrmid
Copy link
Contributor Author

sorry i write down wrongly the name, here the hidde is set to false, even if we pass a hidde = true at the beginning.. So only in the beginning it will hidde the node, but after closing the dropdown this method is triggered removing the hidde = true

@isuscbrmid
Copy link
Contributor Author

I wil test @MJRuskin PR I thing the way he did avoid that overwrite can help me

@isuscbrmid
Copy link
Contributor Author

@mrchief we can close this.. The solution provided by @MJRuskin will work It avoid overwriting the default nodes hide prop when restoring nodes.

@isuscbrmid isuscbrmid closed this May 26, 2020
@isuscbrmid isuscbrmid deleted the enhancement/add-control-prop-to-avoid-navigation-in-node branch May 26, 2020 18:02
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.

4 participants