sadpandajoe opened a new pull request, #34809: URL: https://github.com/apache/superset/pull/34809
<!--- Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/ Example: fix(dashboard): load charts correctly --> ### SUMMARY <!--- Describe the change below, including rationale and design decisions --> Custom icons (Slack, Ballot, etc.) were missing data-test and aria-label attributes while Ant Design icons had them, creating inconsistent accessibility and testing support. This PR fixes the AsyncIcon component to properly pass required props to BaseIconComponent, ensuring all 25+ custom icons have the same attributes as standard icons. #### Problem - Before: Custom icons rendered without accessibility attributes <span role="img"></span> <!-- Missing aria-label and data-test --> - After: Custom icons have proper attributes <span role="img" aria-label="slack" data-test="slack"></span> #### Root Cause AsyncIcon wasn't forwarding the fileName prop to BaseIconComponent, preventing it from generating the required accessibility attributes via the genAriaLabel() function. #### Changes Made 1. AsyncIcon.tsx - Fixed prop forwarding - Explicitly pass fileName, customIcons, iconSize, iconColor, and viewBox to BaseIconComponent - Ensures BaseIcon can generate proper aria-label from fileName 2. Integration Tests - Added real component testing - Created AsyncIcon.integration.test.tsx to test actual component behavior (not mocks) - Verifies all icons have aria-label and data-test attributes - Tests role changes appropriately (img vs button based on onClick) 3. RecipientIcon Tests - Added accessibility verification - Ensures all notification types (Email, Slack, SlackV2) have consistent attributes - Validates that custom icons match Ant Design icon behavior 4. Simplified Mock - Reduced complexity in shim.tsx - Kept mock simple while ensuring it provides essential attributes for tests - Removed overcomplicated aria-label generation logic #### Affected Custom Icons Ballot, BigNumberChartTile, Binoculars, Category, Certified, CheckboxHalf, CheckboxOff, CheckboxOn, CircleSolid, Drag, ErrorSolidSmallRed, Error, Full, Layers, Queued, Redo, Running, Slack, Square, SortAsc, SortDesc, Sort, Transparent, TriangleDown, Undo ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF <!--- Skip this if not applicable --> ### TESTING INSTRUCTIONS <!--- Required! What steps can be taken to manually verify the changes? --> - Integration tests pass for AsyncIcon component - RecipientIcon tests verify all notification types have attributes - Existing tests continue to pass with simplified mock - Manual verification in browser confirms attributes are present - Pre-commit hooks pass (ESLint, TypeScript, formatting) ### ADDITIONAL INFORMATION <!--- Check any relevant boxes with "x" --> <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue --> - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
