Skip to content

feat: Table Column Groups Collection Prefs#4487

Open
NathanZlion wants to merge 5 commits into
mainfrom
dev-v3-natidere-collection-prefs
Open

feat: Table Column Groups Collection Prefs#4487
NathanZlion wants to merge 5 commits into
mainfrom
dev-v3-natidere-collection-prefs

Conversation

@NathanZlion
Copy link
Copy Markdown
Member

@NathanZlion NathanZlion commented May 4, 2026

Description

Add Table Column Grouping Feature support in collection preferences.

Related links, issue AWSUI-9594, if available: n/a

How has this been tested?

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

  • Changes include appropriate documentation updates.
  • Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
  • Changes do not include unsupported browser features, see CONTRIBUTING.md.
  • Changes were manually tested for accessibility, see accessibility guidelines.

Security

Testing

  • Changes are covered with new/existing unit tests?
  • Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@NathanZlion NathanZlion changed the title feat: Add collection preferences for grouped items feat: Table Groups Collection Pres May 4, 2026
@NathanZlion NathanZlion changed the title feat: Table Groups Collection Pres feat: Table Column Groups Collection Pres May 4, 2026
@NathanZlion NathanZlion changed the title feat: Table Column Groups Collection Pres feat: Table Column Groups Collection Prefs May 4, 2026
@NathanZlion NathanZlion force-pushed the dev-v3-natidere-collection-prefs branch from aef643e to d30a4c0 Compare May 4, 2026 15:59
@NathanZlion NathanZlion force-pushed the dev-v3-natidere-collection-prefs branch from 3eb8ca8 to ba91764 Compare May 5, 2026 14:25
@NathanZlion NathanZlion force-pushed the dev-v3-natidere-collection-prefs branch from ba91764 to 8e47ee3 Compare May 5, 2026 14:49
@NathanZlion NathanZlion force-pushed the dev-v3-natidere-collection-prefs branch from 652c9fc to 2b8739b Compare May 5, 2026 18:00
@NathanZlion NathanZlion marked this pull request as ready for review May 6, 2026 08:36
@NathanZlion NathanZlion requested a review from a team as a code owner May 6, 2026 08:36
@NathanZlion NathanZlion requested review from jperals and removed request for a team May 6, 2026 08:36
@codecov
Copy link
Copy Markdown

codecov Bot commented May 8, 2026

Codecov Report

❌ Patch coverage is 98.40000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.41%. Comparing base (f493264) to head (3388a83).

Files with missing lines Patch % Lines
...c/collection-preferences/content-display/index.tsx 96.22% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4487   +/-   ##
=======================================
  Coverage   97.41%   97.41%           
=======================================
  Files         933      933           
  Lines       29595    29681   +86     
  Branches    10757    10791   +34     
=======================================
+ Hits        28831    28915   +84     
- Misses        716      718    +2     
  Partials       48       48           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.


describe('Collection preferences - Grouped Content Display', () => {
test(
'renders group headers and leaf options',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd cover this in unit tests since they're cheaper to run. Is there anything browser-specific that jsdom can't reproduce? Looks like only drag and drop really needs integ tests in this file, the rest could be unit tests

const toggle = firstOption.findVisibilityToggle().findNativeInput();

// Toggle visibility
await page.click(toggle.toSelector());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We typically write an assertion statement after an event


// Order should have changed
const newTexts = await page.getElementsText(options.toSelector());
expect(newTexts).not.toEqual(initialTexts);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest explicitly checking options order after drag and drop and introduce more complex scenarios such as:

  • drag and drop for groups
  • drag and drop for individual items

const wrapper = renderGroupedContentDisplay();
const options = wrapper.findOptions();
// Should render all 4 options (id1 ungrouped + id2, id3 in g1 + id4 in g2)
expect(options.length).toBeGreaterThanOrEqual(4);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to explicitly check the content of the options

// id1 is visible, id2 is visible, id3 is not visible, id4 is visible
const toggleStates = options.map(opt => opt.findVisibilityToggle().findNativeInput().getElement().checked);
// At minimum, not all should be checked (id3 is false)
expect(toggleStates).toContain(false);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same - check explicitly what is inside the array

i18nStrings,
sortDisabled,
}: {
node: OptionTreeNode & { type: 'group' };
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the type: group isn't a part of the OptionTreeNode?

node: OptionTreeNode & { type: 'group' };
onToggle: (id: string) => void;
onChildrenChange: (children: OptionTreeNode[]) => void;
i18nStrings: React.ComponentProps<typeof InternalList>['i18nStrings'];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] you can import underlying interface DndAreaI18nStrings directly from src/internal/components/sortable-area/interfaces.ts

i18nStrings={i18nStrings}
onSortingChange={
// istanbul ignore next: requires DnD interaction
({ detail: { items } }) => onTreeChange([...items])
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can actually test this event same way as in src/file-dropzone/__tests__/file-dropzone.test.tsx

id: node.id,
announcementLabel:
node.type === 'group'
? `${node.label}, ${node.children.length} items`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it come from i18n strings?

const result: string[] = [];
for (const item of items) {
if (item.type === 'group') {
// istanbul ignore next: covered by integration tests
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

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.

2 participants