Deduplicate ActionMenu.tsx

[iansan5653]

While looking into #3712, I noticed that ActionMenu.tsx is duplicated. It exists once in src/ and once in src/ActionMenu. The src/ActionMenu.tsx file is the one being exported because it takes priority over src/ActionMenu/index.ts, but the src/ActionMenu/ActionMenu.tsx file is the one being tested and rendered in Storybook. This strikes me as particularly risky as the two could diverge over time (they already have in that the aria-labelledby prop was only added to the file in src/).

This appears to have been a leftover from some rearranging of files.

This PR cleans up the duplication by merging the two files and deleting the one in src/.

• Closes Remove duplicated ActionMenu file #2992

[iansan5653]

This was the only change that needed to be pulled in from the deleted file

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	100.69 KB (0%)
dist/browser.umd.js	101.27 KB (0%)

[siddharthkp]

That IS very risky!! Looks like src/ActionMenu was only introduced in Feb 2023.

I'm going to compare the two components closely to make sure we haven't been distributing bug fixes between them 💀

[broccolinisoup]

Hi @iansan5653 👋🏻 Thanks so much for pushing this PR! I found that we even had an issue created to delete some duplicate files that includes ActionMenu.tsx - I updated the description of the issue as we revereted some Dialog changes so only file that is duplicated for now is ActionMenu.tsx

Your changes is looking great, my only concern is that there is a manual import at dotcom from a file path. If we could fix that at dotcom before introducing this changes in primer/react side, I think it should be good. Let me know if I am misinterpreting anything!

Feel free to update your description withCloses https://github.com/primer/react/issues/2992 as well.

Thanks again for pushing this PR 🙌🏻

[iansan5653]

my only concern is that there is a manual import at dotcom from a file path

I think this is the import in question:

import {ActionMenu} from '@primer/react/lib-esm/ActionMenu'

This should still work with this change, because it will automatically resolve to ActionMenu/index.ts instead of ActionMenu.tsx now that the file is deleted. This is also why we don't have to change any import paths within primer/react either.

[iansan5653]

The diff is changing as I work! The new one highlights a small difference in ActionMenuButtonProps; easy to resolve.

diff --git a/src/ActionMenu.tsx b/src/ActionMenu/ActionMenu.tsx
index 2f55d433..b84b1a65 100644
--- a/src/ActionMenu.tsx
+++ b/src/ActionMenu/ActionMenu.tsx
@@ -1,14 +1,14 @@
 import React from 'react'
 import {TriangleDownIcon} from '@primer/octicons-react'
-import {AnchoredOverlay, AnchoredOverlayProps} from './AnchoredOverlay'
-import {OverlayProps} from './Overlay'
-import {useProvidedRefOrCreate, useProvidedStateOrCreate, useMenuKeyboardNavigation} from './hooks'
-import {Divider} from './ActionList/Divider'
-import {ActionListContainerContext} from './ActionList/ActionListContainerContext'
-import {Button, ButtonProps} from './Button'
-import {useId} from './hooks/useId'
-import {MandateProps} from './utils/types'
-import {ForwardRefComponent as PolymorphicForwardRefComponent} from './utils/polymorphic'
+import {AnchoredOverlay, AnchoredOverlayProps} from '../AnchoredOverlay'
+import {OverlayProps} from '../Overlay'
+import {useProvidedRefOrCreate, useProvidedStateOrCreate, useMenuKeyboardNavigation} from '../hooks'
+import {Divider} from '../ActionList/Divider'
+import {ActionListContainerContext} from '../ActionList/ActionListContainerContext'
+import {Button, ButtonProps} from '../Button'
+import {useId} from '../hooks/useId'
+import {MandateProps} from '../utils/types'
+import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic'
 
 export type MenuContextProps = Pick<
   AnchoredOverlayProps,
@@ -73,7 +73,7 @@ const Anchor = React.forwardRef<HTMLElement, ActionMenuAnchorProps>(({children,
 })
 
 /** this component is syntactical sugar 🍭 */
-export type ActionMenuButtonProps = ButtonProps & {
+export type ActionMenuButtonProps = Omit<ButtonProps, 'children'> & {
   children: React.ReactNode
 }
 const MenuButton = React.forwardRef(({...props}, anchorRef) => {
@@ -91,12 +91,7 @@ type MenuOverlayProps = Partial<OverlayProps> &
      */
     children: React.ReactNode
   }
-const Overlay: React.FC<React.PropsWithChildren<MenuOverlayProps>> = ({
-  children,
-  align = 'start',
-  'aria-labelledby': ariaLabelledby,
-  ...overlayProps
-}) => {
+const Overlay: React.FC<React.PropsWithChildren<MenuOverlayProps>> = ({children, align = 'start', ...overlayProps}) => {
   // we typecast anchorRef as required instead of optional
   // because we know that we're setting it in context in Menu
   const {anchorRef, renderAnchor, anchorId, open, onOpen, onClose} = React.useContext(MenuContext) as MandateProps<
@@ -124,7 +119,7 @@ const Overlay: React.FC<React.PropsWithChildren<MenuOverlayProps>> = ({
           value={{
             container: 'ActionMenu',
             listRole: 'menu',
-            listLabelledBy: ariaLabelledby || anchorId,
+            listLabelledBy: anchorId,
             selectionAttribute: 'aria-checked', // Should this be here?
             afterSelect: onClose,
           }}

[broccolinisoup]

👋🏻 @iansan5653 integration tests are complaining about not being able to resolve @primer/react/lib-esm/ActionMenu - should we correct the path at dotcom?

cc @siddharthkp as the release coordinator

[siddharthkp]

looking at the logs, and looks like the import reaches out for ./lib-esm/ActionMenu.js instead of ./lib-esm/ActionMenu/index.js 🤔

import {ActionMenu} from '@primer/react/lib-esm/ActionMenu'

Module not found: Error: Can't resolve '@primer/react/lib-esm/ActionMenu' in '/workspace/github/ui/packages/memex/src/client/components/tracked-by'
    
using description file: /workspace/github/node_modules/@primer/react/package.json (relative path: .)
              using exports field: ./lib-esm/ActionMenu.js

It's because ActionMenu/index.js doesn't exist, because rollup doesn't see the need for it 🙃

Looking at some other components, we usually don't have a Component/index.js file in the build, which I think is totally fine given we recommend importing components from the @primer/react root.

Link: https://unpkg.com/browse/@primer/[email protected]/lib-esm/ActionMenu/

We have 3 options:

1. in dotcom, update the imports in dotcom to import {ActionMenu} from '@primer/react' like we recommend
2. in dotcom, update the imports in dotcom to import {ActionMenu} from '@primer/react/lib-esm/ActionMenu/ActionMenu' like we do sometimes for other components like Dialog. I am not aware of the benefits of this.
3. Add some contents to index.ts to convince rollup to generate a index.js which would make this backward compatible.

I think we should time box option 3 and use option 1 as a backup. I can work on this :)

[siddharthkp]

I think we should time box option 3 and use option 1 as a backup. I can work on this

• I have a PR ready for review :) Export ActionMenu/index for backward compatibility with #3713 #3740