-
Notifications
You must be signed in to change notification settings - Fork 9
feat: Adds support for table data grouping #126
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #126 +/- ##
===========================================
+ Coverage 98.59% 100.00% +1.40%
===========================================
Files 16 13 -3
Lines 498 418 -80
Branches 171 156 -15
===========================================
- Hits 491 418 -73
+ Misses 5 0 -5
+ Partials 2 0 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5479f94 to
c646cd2
Compare
package.json
Outdated
| "require": "./cjs/operations.js", | ||
| "default": "./mjs/operations.js" | ||
| }, | ||
| "./internal-do-not-use": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The internal exports are to be consumed by components. The idea is to use the same selection tree util in collection hook and table. In the future, we can also make it public if requested: the tool can help to query selected elements of any subtree etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @cloudscape-design/collection-hooks/internal-do-not-use is not correctly resolved by components with TS unless the types are added to the ./lib folder or tsconfig is changed in components. I chose the former to minimise the changes to components.
| const generateItems = (length: number) => | ||
| Array.from({ length }, (_, index) => ({ id: `${index + 1}` })) as ReadonlyArray<Item>; | ||
|
|
||
| const treeItems = generateItems(25); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed these to use the items array from below in most tests.
| }); | ||
|
|
||
| test('item counts sum up to total count', () => { | ||
| for (let totalItems = 1; totalItems <= 25; totalItems += 5) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a property-based test, which works on a randomly generated tree. We run it multiple times in order to:
- capture more cases (tree small and large, deeply-nested or flat)
- ensure the corner cases have enough chance to occur (during development I run this test 10_000 times, but we don't need as many for the regular runs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is risky as it gives the chance that:
- corner cases are missed, depending on the actual cases tested in a given run, letting bugs potentially pass unnoticed
- we dismiss actual failures as flakiness because they don't happen every time
I think it would be preferable to pre-generate the relevant edge cases so we have more certainty in our test suites.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are proper unit tests for counts and total counts:
- "computes total counts correctly"
- "computes total selected counts correctly"
- "computes item counts correctly"
- "computes selected item counts correctly"
The property-based tests ("item counts sum up to total count" and "total count equals items size when dataGrouping=undefined") are there for extra safety against existing or future corner cases that I could have missed. These cannot be flaky: if such test fails, it would mean that there is a logical bug that must be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but the random nature of the test data means we may not always test all cases. So if a specific test run only includes "good" data, it will pass. I agree that a fail in these tests would always be a valid fail, but a pass would not always be a valid pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gethinwebster that is true. As a compromise solution, I can increase the number of test samples so that the likelihood of these tests failing timely increases, too. I did run them 10_000 times when testing locally, but that many runs would not be justified for the pipeline. It is also hard to replace these tests with dedicated test scenarios as I can't think of any that are not already covered by the four unit tests I mentioned above. At the same time, the data structure complexity leaves a chance that some use cases were missed or can get missed should the future implementation change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, random data should only be used for testing when its random nature is essential/has some impact on the underlying code. In this case, that's not true: the underlying code could reasonably be analyzed and appropriate test cases built for it.
At the very least I would recommend renaming generateNestedItems -> generateRandomNestedItems so it's clear from the test what is happening. And perhaps a code comment making it clear why it's tested in this way. I think this is an extreme outlier in terms of testing strategy, and if you're certain it's necessary we should make it clear that it's a non-standard approach and not something we would recommend in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such tests are used a good number of times in the board components, see for example: https://github.com/cloudscape-design/board-components/blob/main/src/internal/layout-engine/__tests__/engine-move-blocks.test.ts#L10
I will rename the util
| onExpandableItemToggle(event: CustomEventLike<{ item: T; expanded: boolean }>): void; | ||
| // The groupSelection property is only added in case selection is configured, and expandableRows.dataGrouping=true. | ||
| groupSelection?: GroupSelectionState<T>; | ||
| onGroupSelectionChange(event: CustomEventLike<GroupSelectionChangeDetail<T>>): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The event handler, though, is always defined to avoid unnecessary conditional code on the consumer's end. When this function is called when group selection is not used - the internal state will be set, but w/o any effects to the UI.
| return itemCount; | ||
| } | ||
| for (const item of items) { | ||
| totalItemsCount += computeCounts(item); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The totalItemsCount computation was altered as now it is different depending on the dataGrouping flag. For dataGrouping=false it still includes every item, which is validated with a test "total count equals items size".
c646cd2 to
e992a7a
Compare
src/operations/selection-tree.ts
Outdated
| trackBy?: TrackBy<T>; | ||
| } | ||
|
|
||
| export class SelectionTree<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This util was originally introduced in the POC for table grouping: cloudscape-design/components#3673
However, we need it in both collection hooks (to initialise from defaultSelectedItems, compute selected counts, etc.) and table (to perform updates). To reuse the code, the util is introduced here and is shared as internal-do-not-use for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider adding this to component-toolkit instead?
This way we keep the collection hooks package a bit more separated from components, and we don't need to do package export configuration changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the package configuration changes are unfortunate - I agree.
At the same time, we already export some utilities from the collection-hooks (the "operations" ns) - that are related to the collection computations. I find the selection tree the same. For instance, it reuses the TrackBy type and utility - that would need to be also copied/moved to the toolkit if we decide so.
I propose that we leave this as is for now, but keep your idea in mind. Say, if we want to reuse the util at some point with other components like multiselect or tree-view, or expose it publicly - then moving it to the toolkit can be well justified.
src/types.ts
Outdated
| declare global { | ||
| const process: { env: { NODE_ENV?: string } }; | ||
| const console: { warn: (...args: Array<any>) => void }; | ||
| const console: { log: (...args: Array<any>) => void; warn: (...args: Array<any>) => void }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows code be compiled with console.log() statements added - which is annoying otherwise.
src/use-collection-state.ts
Outdated
| filteringText: options.filtering?.defaultFilteringText ?? '', | ||
| propertyFilteringQuery: options.propertyFiltering?.defaultQuery ?? { tokens: [], operation: 'and' }, | ||
| groupSelection: options.selection?.defaultSelectedItems | ||
| ? { baseline: 'none', toggledItems: options.selection.defaultSelectedItems } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The groupSelection.toggledItems can be initialised with normal selection's defaultSelectedItems just fine. In that case, the grouped nature of the data is assumed - meaning that only leaf nodes are considered selectable.
bae9040 to
020c44d
Compare
| provider: 'istanbul', | ||
| include: ['src/**'], | ||
| exclude: ['**/debug-tools/**', '**/test/**'], | ||
| exclude: ['**/__tests__/**'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was misconfigured: there are no **/test or **/debug-tools paths in the project. At the same time, the __tests__ path was not included and thus the test helpers did contribute to coverage mistakingly.
8edd3e8 to
2232e0a
Compare
|
|
||
| // There is no configuration for data grouping yet, but it might come in future releases. | ||
| // eslint-disable-next-line @typescript-eslint/no-empty-object-type | ||
| export interface DataGroupingProps {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] do we want to add something like active: true? I'm not totally averse to empty objects turning on a feature, but it does look a little bit strange
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gethinwebster the reason it is implemented this way - is because we already use this approach for other settings in collection hooks, kind of. Say, these two examples are different (docs):
// The collectionProps does not include sorting and selection state/handlers.
const { items, collectionProps } = useCollection(allItems, {});
// The collectionProps does include sorting and selection state/handlers.
const { items, collectionProps } = useCollection(allItems, { sorting: {}, selection: {} });
I was thinking of making the type {} | boolean - so that one can do expandableRows.dataGrouping = true. This is better ergonomic than including a nested prop - but, again, inconsistent with what we have today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the existing usage is why I don't think it's blocking. It's a bit different because we don't have any other properties within the object (yet) though. I quite like {} | boolean, but happy with whatever here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can do {} | boolean but then also update other settings to support the same, which is also possible as a follow-up change.
1b18047 to
e46ca14
Compare
e46ca14 to
410913a
Compare
60dd0f1 to
d61b09e
Compare
| declare global { | ||
| const process: { env: { NODE_ENV?: string } }; | ||
| const console: { warn: (...args: Array<any>) => void }; | ||
| type AbortSignal = any; // Used in the component-toolkit dependency |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate on why this is necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In collection hooks we do not use "dom" types in tsconfig lib, see: https://github.com/cloudscape-design/collection-hooks/pull/91/files#r1904006300
As result, using global APIs such as process, console, or AbortSignal requires explicit declarations. The AbortSignal used inside the component-toolkit, and since we don't skip lib checks - the collection hooks build fails because of that, unless the type is added.
scripts/prepare-package-lock.js
Outdated
| @@ -0,0 +1,36 @@ | |||
| #!/usr/bin/env node | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add it to the build-tools package instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of doing that as a follow up, since there are slightly different versions of this script across packages, but it should be easy to do it right away - I will take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Adds support for table data grouping and grouped selection.
Required for cloudscape-design/components#3673, API: [WCacAJiTuIc6]
API changes
The collection hooks received support for expandable tables with grouped data, which affects counters and selected state computations.
The use-collection options were extended with the ability to specify
dataGroupingwhen configuring expandable rows - this changes how counters and selection work. See examples 1-3 below.The table above does not feature selection, but the
totalItemsCounteris computed differently and only includes the leaf nodes.The table above will have group selection active even though selectionType ("single" | "multi") is not set - it works because the
collectionProps.expandableRows.groupSelectionis provided by collection hooks. The table will also show per-item total and selection counters, represented ascollectionProps.expandableRows.getItemsCountandcollectionProps.expandableRows.getSelectedItemsCount.The specified items
item1anditem2will be selected by default, same as it works with normal selection. The default state is transformed into group selection state{ inverted: false, toggledItems: [item1, item2] }.When using data grouping, the
collectionProps.selectedItemsis derived from the collection state and includes all logically selected leaf nodes. The consumers do not have to use the more complexcollectionProps.expandableRows.groupSelectionrepresentation, used by the table component to solve for partial data use cases.