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

fix(taxonomy tree): options remain selected when switching in single mode #1765

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

JakeStanger
Copy link
Contributor

@JakeStanger JakeStanger commented Feb 14, 2024

Q A
Bug fix? [x]
New feature? [ ]
New sample? [ ]
Related issues?

What's in this Pull Request?

This is a hack fix since the tree is already built on top of React anti-patterns, which cause this in the first place. In the future a full refactor is really required to sort this out at its roots.

By adding an unstable key, it forces the component to re-render on every render. Without this, React assumes nothing has changed and does not react to the selectedKey update.

For large trees with everything expanded, this could cause performance issues but should be okay in most cases.

…mode

This is a hack fix since the tree is already built on top of React anti-patterns, which cause this in the first place. In the future a full refactor is really required to sort this out at its roots.

By adding an unstable `key`, it forces the component to re-render on every render. Without this, React assumes nothing has changed and does not react to the `selectedKey` update.

For large trees with everything expanded, this could cause performance issues but should be okay in most cases.
@JakeStanger JakeStanger force-pushed the fix/taxonomy-tree-selection branch from 9c28519 to c358b58 Compare February 14, 2024 15:24
@joelfmrodrigues
Copy link
Collaborator

Hi @JakeStanger many thanks for the update. To be completely sure that we don't break things for other developers already using the component that may have a very large collection, and since there is a small risk involved, could you please add a new property to the component to control enabling this behaviour (disabled by default)?

@AJIXuMuK
Copy link
Collaborator

@JakeStanger - would it be sufficient to generate a key based on some set of props?
For example, if for now we're fixing allowMultipleSelections - just use

key={(!!allowMultipleSelections).toString()}

@JakeStanger
Copy link
Contributor Author

To be completely sure that we don't break things for other developers already using the component that may have a very large collection, and since there is a small risk involved, could you please add a new property to the component to control enabling this behaviour (disabled by default)?

I disagree with the idea that a fix should be opt-in. The current behaviour is broken, in that the UI does not fully update when changing selection within the panel. This should not be an available behaviour in any configuration. If performance is a concern (really this needs properly testing, as I'm just assuming) that needs to be properly addressed rather than adding yet another bodge to the pile.

would it be sufficient to generate a key based on some set of props?

The only prop should matter here is selectedKey going into the ChoiceGroup since that's the one causing the issue, due to the whole storing-props-as-state ordeal that seemingly both Fluent UI and this component are doing. It may therefore be sufficient to provide this as a key, but I'm not sure (if using this as a key is enough, why isn't React responding to the prop change already?). That would therefore need to be tested.

The bug doesn't arise from a change in props into the component so it wouldn't make sense to use these. This is why I've opted for an unstable value, as a re-render is needed due to an internal state change that the Fluent component doesn't correctly respond to (partially due to its misuse).


For clarity, here's rough repro steps I should have included before:

  1. Add taxonomy tree component in single choice mode
  2. In browser, open selection panel and select a top level option.
  3. While panel is still open, select another top level option.
  4. Observe both show as selected.

I am on holiday this week but can make any required changes next week.

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