1328567 - TreeView expandedNodes should update when props changed

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect, P1)

Description

[Ricky Chien [:rickychien] (inactive)]

I ran into this issue when using TreeView in ResponsePanel (bug 1317651).

Current TreeView will accept an expandedNodes prop in getInitialState:

expandedNodes: this.props.expandedNodes,

Expanded nodes will not be updated after component mount if we pass new expandedNodes prop into TreeView because there is no way to update this.state.expandedNodes state.

It's a necessary since ResponsePanel will update TreeView object prop along with new expandedNodes prop depends on selected request. So we should fix this by triggering a setState in componentWillReceiveProps.

Comment 1

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1328567 - TreeView expandedNodes should update when props changed

Review commit: https://reviewboard.mozilla.org/r/102114/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102114/

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8823576 [details]
Bug 1328567 - TreeView expandedNodes should update when props changed

https://reviewboard.mozilla.org/r/102114/#review102462

::: devtools/client/shared/components/tree/tree-view.js:132
(Diff revision 1)
> +        this.setState(Object.assign({}, this.state, {
> +          expandedNodes: new Set([...nodes, ...expandedNodes]),
> +        }));

I think it should be rather:

this.setState(Object.assign({}, this.state, {
  expandedNodes,
}));

The consumer should be responsible for joining previously expanded nodes with new set of nodes if needed.

Does that work for your case?

Honza

Comment 3

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8823576 [details]
Bug 1328567 - TreeView expandedNodes should update when props changed

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102114/diff/1-2/

Comment 4

[Ricky Chien [:rickychien] (inactive)]

Yes, it works for my case! I updated my patch, please take a look again.

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8823576 [details]
Bug 1328567 - TreeView expandedNodes should update when props changed

https://reviewboard.mozilla.org/r/102114/#review102558

R+, thanks!

Honza

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4903a05dfaec
TreeView expandedNodes should update when props changed r=Honza

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4903a05dfaec