Skip to content

feat: add dev warning for missing field definition labels in attribute-editor#4337

Open
Who-is-PS wants to merge 6 commits intomainfrom
dev-v3-philosr-AWSUI-61432
Open

feat: add dev warning for missing field definition labels in attribute-editor#4337
Who-is-PS wants to merge 6 commits intomainfrom
dev-v3-philosr-AWSUI-61432

Conversation

@Who-is-PS
Copy link
Member

Description

Adds a console warning via warnOnce when an AttributeEditor field definition is missing a label prop. The label is used as aria-label for the inputs in the column, so omitting it degrades the screen reader experience.

This change helps guide developers toward providing accessible labels without introducing a breaking change (making label required).

Related links, issue #, if available: AWSUI-61432

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.

@codecov
Copy link

codecov bot commented Mar 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.44%. Comparing base (7d1f303) to head (d121224).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4337   +/-   ##
=======================================
  Coverage   97.44%   97.44%           
=======================================
  Files         897      897           
  Lines       26397    26401    +4     
  Branches     9527     9528    +1     
=======================================
+ Hits        25723    25727    +4     
  Misses        631      631           
  Partials       43       43           

☔ 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.

@Who-is-PS Who-is-PS marked this pull request as ready for review March 9, 2026 18:31
@Who-is-PS Who-is-PS requested a review from a team as a code owner March 9, 2026 18:31
@Who-is-PS Who-is-PS requested review from cansuaa and taheramr and removed request for a team March 9, 2026 18:31

jest.mock('@cloudscape-design/component-toolkit', () => ({
...jest.requireActual('@cloudscape-design/component-toolkit'),
useContainerQuery: jest.fn().mockImplementation(() => ['m', () => {}]),
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this line? If you mock '@cloudscape-design/component-toolkit/internal' we shouldn't need this since we are only interested in 'warnOnce'. I'd check how the other warnOnce usages are tested e.g.

jest.mock('@cloudscape-design/component-toolkit/internal', () => ({
...jest.requireActual('@cloudscape-design/component-toolkit/internal'),
warnOnce: jest.fn(),
}));
afterEach(() => {
(warnOnce as jest.Mock).mockReset();
});

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah true, the useContainerQuery mock is unnecessary here. I've switched to mocking @cloudscape-design/component-toolkit/internal with just warnOnce: jest.fn(), matching the pattern in button-dropdown and table. This also lets us drop the consoleWarnSpy entirely since we can assert directly on the warnOnce mock and use mockReset() between tests instead of relying on a separate file for a clean warnOnce cache

@Who-is-PS Who-is-PS requested a review from cansuaa March 10, 2026 14:14
/>
);

expect(warnOnce).toHaveBeenCalledTimes(1);
Copy link
Member

Choose a reason for hiding this comment

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

Could you check why it was being triggered twice before deleting it?

Copy link
Member Author

Choose a reason for hiding this comment

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

The warning check runs directly in the render body (not in a useEffect/useMemo), so it fires on every render. The original test had useContainerQuery mocked to return a stable value (['m', () => {}]), which prevented the re-render. When I refactored the test to mock @cloudscape-design/component-toolkit/internal instead of @cloudscape-design/component-toolkit, I lost that useContainerQuery mock — so the real hook runs, resolves the container query asynchronously, and triggers a second render → second warnOnce call.

@Who-is-PS Who-is-PS added this pull request to the merge queue Mar 12, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Mar 12, 2026
@Who-is-PS Who-is-PS added this pull request to the merge queue Mar 12, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 12, 2026
@Who-is-PS Who-is-PS added this pull request to the merge queue Mar 12, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 12, 2026
@Who-is-PS Who-is-PS enabled auto-merge March 12, 2026 14:52
@Who-is-PS Who-is-PS added this pull request to the merge queue Mar 12, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 12, 2026
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