search

iTwin / presentation

Monorepo for iTwin.js Presentation Library
https://www.itwinjs.org/presentation/
MIT License
4 stars 0 forks source link

Hierarchies-react: pass node to `onNodeClick` instead of node ID #615

Closed jasdom closed 3 months ago

jasdom commented 3 months ago

Changes needed for https://github.com/iTwin/viewer-components-react/issues/867. Also memoized the onNodeClick and onNodeKeyDown as TreeRenderer contains a hook that depends on them.

changeset-bot[bot] commented 3 months ago

🦋 Changeset detected

Latest commit: dfe160b3e14b3a492dd3ccd94594d787b91b8b9a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | ------------------------------------- | ----- | | @itwin/presentation-hierarchies-react | Minor |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

jasdom commented 3 months ago

We have other APIs (e.g. see UseTreeResult), which work with node ids rather than nodes. So this PR kind of moves away from that pattern - why? How likely is it that we'll need to modify those other APIs to work with nodes rather than ids?

When only the id is passed the consumer has to either find a way to retrieve the data for the node (for example if they need access to extendedData) or wrap the callback manually to include the node which is inconvenient. I personally would be for passing nodes instead of ids in other APIs as well as it is more future-proof and easier to use. We also aready pass the entire node to getIcon and getSublabel so that pattern was already broken.

saskliutas commented 3 months ago

We have other APIs (e.g. see UseTreeResult), which work with node ids rather than nodes. So this PR kind of moves away from that pattern - why? How likely is it that we'll need to modify those other APIs to work with nodes rather than ids?

When only the id is passed the consumer has to either find a way to retrieve the data for the node (for example if they need access to extendedData) or wrap the callback manually to include the node which is inconvenient. I personally would be for passing nodes instead of ids in other APIs as well as it is more future-proof and easier to use. We also aready pass the entire node to getIcon and getSublabel so that pattern was already broken.

I think all callbacks that are passed to TreeRenderer/TreeNodeRenderer (related to rendering nodes) should get node instead of nodeId. I don't think there is need to change functions retuned by useTree to take nodes instead of ids. They usually will be called from UI component event handlers so consumer should have access to the node before calling them.

grigasp commented 3 months ago

We have other APIs (e.g. see UseTreeResult), which work with node ids rather than nodes. So this PR kind of moves away from that pattern - why? How likely is it that we'll need to modify those other APIs to work with nodes rather than ids?

When only the id is passed the consumer has to either find a way to retrieve the data for the node (for example if they need access to extendedData) or wrap the callback manually to include the node which is inconvenient. I personally would be for passing nodes instead of ids in other APIs as well as it is more future-proof and easier to use. We also aready pass the entire node to getIcon and getSublabel so that pattern was already broken.

I think all callbacks that are passed to TreeRenderer/TreeNodeRenderer (related to rendering nodes) should get node instead of nodeId. I don't think there is need to change functions retuned by useTree to take nodes instead of ids. They usually will be called from UI component event handlers so consumer should have access to the node before calling them.

Sounds good. @jasdom, please also change TreeNodeRendererOwnProps.onFilterClick to take node instead of id.

jasdom commented 3 months ago

We have other APIs (e.g. see UseTreeResult), which work with node ids rather than nodes. So this PR kind of moves away from that pattern - why? How likely is it that we'll need to modify those other APIs to work with nodes rather than ids?

When only the id is passed the consumer has to either find a way to retrieve the data for the node (for example if they need access to extendedData) or wrap the callback manually to include the node which is inconvenient. I personally would be for passing nodes instead of ids in other APIs as well as it is more future-proof and easier to use. We also aready pass the entire node to getIcon and getSublabel so that pattern was already broken.

I think all callbacks that are passed to TreeRenderer/TreeNodeRenderer (related to rendering nodes) should get node instead of nodeId. I don't think there is need to change functions retuned by useTree to take nodes instead of ids. They usually will be called from UI component event handlers so consumer should have access to the node before calling them.

Sounds good. @jasdom, please also change TreeNodeRendererOwnProps.onFilterClick to take node instead of id.

In some cases the onFilterClick callback uses the parent node id so providing a node was not an option. Instead switched to passing HierarchyLevelDetails as the node id was only used for retrieving the details.

github-actions[bot] commented 3 months ago

Unified selection benchmark

Benchmark suite Current: dfe160b3e14b3a492dd3ccd94594d787b91b8b9a Previous: bda5efff29313d6931dbe128db1f4c678f7217f8 Deviation Status
hilite 50k elements 1175.51 ms 1305.69 ms -9.9702% ✅
hilite 50k elements (P95 of main thread blocks) 50 ms 56 ms -10.7143% ✅
hilite 50k group elements 240.29 ms 245.99 ms -2.3172% ✅
hilite 50k group elements (P95 of main thread blocks) 31 ms 32 ms -3.1250% ✅
hilite 1k subjects 47693.44 ms 46528.72 ms 2.5032% 🚨
hilite 1k subjects (P95 of main thread blocks) 31 ms 36 ms -13.8889% ✅
hilite 50k subcategories 285.68 ms 285.15 ms 0.1859% 🚨
hilite 50k subcategories (P95 of main thread blocks) 33 ms 33 ms 0% 🟰
hilite 50k functional 3D elements 26676.44 ms 25765.77 ms 3.5344% 🚨
hilite 50k functional 3D elements (P95 of main thread blocks) 37 ms 40 ms -7.5000% ✅
hilite 50k functional 2D elements 6233.28 ms 6273.64 ms -0.6433% ✅
hilite 50k functional 2D elements (P95 of main thread blocks) 31 ms 40 ms -22.5000% ✅
compute selection for 50k elements 303.34 ms 312.38 ms -2.8939% ✅
compute selection for 50k elements (P95 of main thread blocks) 33 ms 31 ms 6.4516% 🚨
compute parent selection for 50k elements 360.06 ms 349.91 ms 2.9007% 🚨
compute parent selection for 50k elements (P95 of main thread blocks) 31 ms 31 ms 0% 🟰
compute top ancestor selection for 50k elements 581.67 ms 587.02 ms -0.9114% ✅
compute top ancestor selection for 50k elements (P95 of main thread blocks) 0 ms 0 ms NaN% 🚨
compute category selection for 50k elements 91.41 ms 95.36 ms -4.1422% ✅
compute category selection for 50k elements (P95 of main thread blocks) 0 ms 0 ms NaN% 🚨
compute model selection for 50k elements 75.2 ms 87.58 ms -14.1356% ✅
compute model selection for 50k elements (P95 of main thread blocks) 0 ms 0 ms NaN% 🚨
compute functional selection for 50k 3D elements 401.66 ms 407.82 ms -1.5105% ✅
compute functional selection for 50k 3D elements (P95 of main thread blocks) 31 ms 31 ms 0% 🟰
compute parent functional selection for 50k 3D elements 443.37 ms 451.15 ms -1.7245% ✅
compute parent functional selection for 50k 3D elements (P95 of main thread blocks) 31 ms 31 ms 0% 🟰
compute top ancestor functional selection for 50k 3D elements 1178.25 ms 1247.03 ms -5.5155% ✅
compute top ancestor functional selection for 50k 3D elements (P95 of main thread blocks) 0 ms 0 ms NaN% 🚨
compute functional selection for 50k 2D elements 3121.71 ms 3195.14 ms -2.2982% ✅
compute functional selection for 50k 2D elements (P95 of main thread blocks) 0 ms 0 ms NaN% 🚨
compute parent functional selection for 50k 2D elements 3097.19 ms 3270.98 ms -5.3131% ✅
compute parent functional selection for 50k 2D elements (P95 of main thread blocks) 0 ms 0 ms NaN% 🚨
compute top ancestor functional selection for 50k 2D elements 3251.64 ms 3278.91 ms -0.8317% ✅
compute top ancestor functional selection for 50k 2D elements (P95 of main thread blocks) 0 ms 0 ms NaN% 🚨

This comment was automatically generated by workflow using github-action-benchmark.

github-actions[bot] commented 3 months ago

Hierarchies benchmark

Benchmark suite Current: dfe160b3e14b3a492dd3ccd94594d787b91b8b9a Previous: cbf3163d1d50779d9f819f799dbb7de90f2b12bd Deviation Status
flat 50k elements list 4123.29 ms 4084.08 ms 0.9601% 🚨
flat 50k elements list (P95 of main thread blocks) 73 ms 72 ms 1.3889% 🚨
grouping by label 10096.35 ms 10047.76 ms 0.4836% 🚨
grouping by label (P95 of main thread blocks) 56 ms 64 ms -12.5000% ✅
grouping by class 10283.45 ms 10087.38 ms 1.9437% 🚨
grouping by class (P95 of main thread blocks) 44 ms 42 ms 4.7619% 🚨
grouping by property 10720.29 ms 10618.72 ms 0.9565% 🚨
grouping by property (P95 of main thread blocks) 47 ms 53 ms -11.3208% ✅
grouping by base class (10 classes) 7457 ms 7381.67 ms 1.0205% 🚨
grouping by base class (10 classes) (P95 of main thread blocks) 77 ms 84 ms -8.3333% ✅
grouping by multiple attributes 27695.28 ms 27750.24 ms -0.1981% ✅
grouping by multiple attributes (P95 of main thread blocks) 72 ms 41 ms 75.6098% 🚨
hide if no children required to finalize root, w/o children 45972.48 ms 45861.72 ms 0.2415% 🚨
hide if no children required to finalize root, w/o children (P95 of main thread blocks) 37 ms 38 ms -2.6316% ✅
hide if no children required to finalize root, w/ children 158.29 ms 167.33 ms -5.4025% ✅
hide if no children required to finalize root, w/ children (P95 of main thread blocks) 0 ms 0 ms NaN% 🚨
models tree initial (Baytown) 54.22 ms 53.87 ms 0.6497% 🚨
models tree initial (Baytown) (P95 of main thread blocks) 0 ms 0 ms NaN% 🚨
models tree full (Baytown) 7602.93 ms 7562.19 ms 0.5387% 🚨
models tree full (Baytown) (P95 of main thread blocks) 89 ms 92 ms -3.2609% ✅

This comment was automatically generated by workflow using github-action-benchmark.