pnp / sp-dev-fx-controls-react

Reusable React controls for SPFx solutions
https://pnp.github.io/sp-dev-fx-controls-react/
MIT License
380 stars 379 forks source link

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

Open JakeStanger opened 4 months ago

JakeStanger commented 4 months ago
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.

joelfmrodrigues commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

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.