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

feat(tree): improve Tree Data speed considerably, fixes #307 #336

Merged
merged 23 commits into from
May 12, 2021

Conversation

ghiscoding
Copy link
Owner

@ghiscoding ghiscoding commented May 8, 2021

TODOs

  • refactor flat to tree structure and vice versa to optimize speed
    • so far it seems to be ~40-50x faster 😮🚀 (previous code to convert from tree to flat was extremely slow, deep copy and item search were the main culprit).
    • with 100k rows test after sorting "Title" column
      • (before) 3354.64892578125 ms
      • (after) 70.06689453125 ms
  • add ways to change dataset dynamically in the demo (2 buttons to change from 500 to 50k rows)
    • use 500 and 25k instead since 50k is a bit excessive for a demo
  • throw error when trying to use Tree Data without filtering enabled (this is required for Tree Data)
  • we shouldn't sort the dataset if user didn't provide the initialSort property
    • when not provided that means it's assuming the user already sorted the dataset, possibly while querying it from the dataset. However this is not recommended to do, we still have that option, technically speaking Example 5 is already sorted because of how it is constructed).
  • remove previous throw error of multi-columns sort, it seems to work after all (need to test a bit more)
    • it seems to be broken after all, so we should probably leave it as unsupported
  • rename some of the methods, the word "hierarchical" can simply be renamed to "tree"
  • update all unit tests
  • try adding more Cypress E2E tests to cover at least insert of items
    • test that clear filters shows all original items count
  • remove all benchmark console logs before merging the PR
  • add optional prefixes to the Tree Formatter rows
  • test that we should still be able to add new item in both Example 5 and 6
  • add pub/sub events (onBeforeSortChange, onBeforeFilterChange, onBeforeCollapse, ...) to help display spinner when dealing
  • when calling clearFilters remove possibly unnecessary dataView.refresh(), need to test a bit more with backend service as well
  • adding new item should also update formatter on Title column but it doesn't currently do that
  • add SASS vertical alignment variable for the tree collapsing icons
  • add custom title formatter in tree data options for the tree formatter to process

@codecov
Copy link

codecov bot commented May 9, 2021

Codecov Report

Merging #336 (a67f2f3) into master (3e059dc) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #336   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          213       212    -1     
  Lines        13063     13110   +47     
  Branches      4279      4297   +18     
=========================================
+ Hits         13063     13110   +47     
Impacted Files Coverage Δ
packages/common/src/services/index.ts 100.00% <ø> (ø)
...ages/common/src/extensions/contextMenuExtension.ts 100.00% <100.00%> (ø)
...ckages/common/src/formatters/formatterUtilities.ts 100.00% <100.00%> (ø)
...kages/common/src/formatters/treeExportFormatter.ts 100.00% <100.00%> (ø)
packages/common/src/formatters/treeFormatter.ts 100.00% <100.00%> (ø)
packages/common/src/services/filter.service.ts 100.00% <100.00%> (ø)
packages/common/src/services/grid.service.ts 100.00% <100.00%> (ø)
packages/common/src/services/pubSub.service.ts 100.00% <100.00%> (ø)
packages/common/src/services/sort.service.ts 100.00% <100.00%> (ø)
packages/common/src/services/treeData.service.ts 100.00% <100.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e059dc...a67f2f3. Read the comment docs.

ghiscoding added 20 commits May 9, 2021 19:36
- most framework requires one cycle for the binding to be executed and because of that the spinner was showing too late, we can fix this by having the pubsub publish method to return a promise with setTimeout of 0 (which is 1 cycle) and now the spinner works as intended for all events recently added
- this basically runs a Formatter into the Tree Formatter
- also remove `indentedChildValuePrefix` that can easily be done in this new titleFormatter
@ghiscoding ghiscoding changed the title WIP - feat(tree): improve Tree Data speed considerably, fixes #307 feat(tree): improve Tree Data speed considerably, fixes #307 May 12, 2021
@ghiscoding ghiscoding merged commit c22165b into master May 12, 2021
@ghiscoding ghiscoding deleted the feat/tree-data-optimization branch May 12, 2021 00:30
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.

Tree Sort on first page load is incorrect
1 participant