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

refactor(OverflowMenu): convert to functional component and add types #18690

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

adamalston
Copy link
Contributor

Closes #18689

Converted OverflowMenu to a functional component and added types.

Changelog

New

  • Added types to OverflowMenu and other components and utils.

Changed

  • Converted OverflowMenu to a functional component.

Testing / Reviewing

yarn test packages/react/src/components/OverflowMenu

Reference commits: d4a071a...adamalston:carbon:f9390189887725ed4fbe2d1649790861c2bede9b

The changes in this pull request depend on the changes in #18683.

I marked the ariaLabel prop in OverflowMenu.tsx as optional so that the OverflowMenu in TableToolbarMenu.tsx wouldn't throw a type error. The alternative was adding an ariaLabel prop to that OverflowMenu. However, that wouldn't work because 29 DataTable tests would fail due to the warning that's logged when deprecated props are used in tests. You can see for yourself by making the following change and running yarn test packages/react/src/components/DataTable:

diff --git a/packages/react/src/components/DataTable/TableToolbarMenu.tsx b/packages/react/src/components/DataTable/TableToolbarMenu.tsx
index cd6dd8e45..136e3b276 100644
--- a/packages/react/src/components/DataTable/TableToolbarMenu.tsx
+++ b/packages/react/src/components/DataTable/TableToolbarMenu.tsx
@@ -59,6 +59,7 @@ const TableToolbarMenu: React.FC<TableToolbarMenuProps> = ({
   return (
     <OverflowMenu
       aria-label={iconDescription}
+      ariaLabel={iconDescription}
       renderIcon={renderIcon}
       className={toolbarActionClasses}
       title={iconDescription}

Disregarding the reasoning above, the change to mark the prop as optional seems correct because it doesn't make sense for a deprecated prop to be required. This change also demonstrates that the old types for the OverflowMenu component weren't being applied because if they were, there would have been a type error in TableToolbarMenu.tsx.

Copy link

netlify bot commented Feb 25, 2025

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit b9874d1
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/67bdd5ac16b9ce00088ba2ca
😎 Deploy Preview https://deploy-preview-18690--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Feb 25, 2025

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit b9874d1
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/67bdd5ac039fb800080acb26
😎 Deploy Preview https://deploy-preview-18690--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Feb 25, 2025

Deploy Preview for v11-carbon-web-components ready!

Name Link
🔨 Latest commit b9874d1
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-web-components/deploys/67bdd5ac0427cf00084a35b9
😎 Deploy Preview https://deploy-preview-18690--v11-carbon-web-components.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Feb 25, 2025

Codecov Report

Attention: Patch coverage is 86.95652% with 18 lines in your changes missing coverage. Please review.

Project coverage is 84.32%. Comparing base (d4a071a) to head (b9874d1).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...react/src/components/OverflowMenu/OverflowMenu.tsx 86.36% 18 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #18690      +/-   ##
==========================================
- Coverage   84.34%   84.32%   -0.03%     
==========================================
  Files         409      409              
  Lines       14651    14650       -1     
  Branches     4794     4819      +25     
==========================================
- Hits        12358    12354       -4     
- Misses       2131     2135       +4     
+ Partials      162      161       -1     

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

@adamalston adamalston marked this pull request as ready for review February 25, 2025 15:03
@adamalston adamalston requested review from a team as code owners February 25, 2025 15:03
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.

Convert OverflowMenu to a functional component and improve types
1 participant