-
Notifications
You must be signed in to change notification settings - Fork 610
chore(ActionMenu): Add fullscreen sample story #6108
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 51b85aa The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
size-limit report 📦
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This patch introduces a new fullscreen
variant for the ActionMenu
component and adds a sample Storybook story demonstrating fullscreen behavior in narrow viewports.
- Exposes the underlying
AnchoredOverlay
variant
prop onActionMenu.Overlay
with a default value - Defines a
FullScreen
example story showing how to toggle between anchored and fullscreen variants - Updates the changelog for a patch release
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
packages/react/src/ActionMenu/ActionMenu.tsx | Added variant to MenuOverlayProps , set default variant |
packages/react/src/ActionMenu/ActionMenu.examples.stories.tsx | Added FullScreen story showcasing the new fullscreen variant |
.changeset/pink-trees-work.md | Bumped patch release and updated changelog entry |
Comments suppressed due to low confidence (2)
packages/react/src/ActionMenu/ActionMenu.examples.stories.tsx:111
- Consider adding automated tests (unit, snapshot, or visual regression) for the new fullscreen variant to ensure it renders correctly across viewports and prevent regressions.
export const FullScreen = () => {
packages/react/src/ActionMenu/ActionMenu.examples.stories.tsx:127
- [nitpick] Multiple list items share the label 'Option 2', which may confuse readers. Consider using unique labels (e.g., 'Option 3', 'Option 4') to clearly demonstrate multiple entries.
<ActionList.Item>Option 2</ActionList.Item>
@@ -242,6 +242,7 @@ const Overlay: React.FC<React.PropsWithChildren<MenuOverlayProps>> = ({ | |||
side, | |||
onPositionChange, | |||
'aria-labelledby': ariaLabelledby, | |||
variant = {regular: 'anchored', narrow: 'anchored'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defining the default variant
object inline recreates a new reference on every render, which can cause unnecessary re-renders of AnchoredOverlay
. Consider moving the default into a const
outside the component to keep the reference stable.
Copilot uses AI. Check for mistakes.
@@ -229,7 +229,7 @@ const MenuButton = React.forwardRef(({...props}, anchorRef) => { | |||
}) as PolymorphicForwardRefComponent<'button', ActionMenuButtonProps> | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The newly added variant
prop should be documented (e.g., in JSDoc or PropTypes) to clarify the allowed shapes and describe its behavior for maintainers and consumers.
/** | |
* Props for the `Overlay` component used in the `ActionMenu`. | |
* | |
* @property variant - Determines the style of the overlay. It is an object with two optional keys: | |
* - `regular`: Specifies the variant for regular screens. Default is `'anchored'`. | |
* - `narrow`: Specifies the variant for narrow screens. Default is `'anchored'`. | |
* Example: `{regular: 'anchored', narrow: 'floating'}`. | |
*/ |
Copilot uses AI. Check for mistakes.
👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/379309 |
🟢 golden-jobs completed with status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good, but we should definitely add a "Close" button.
There are two ways we can do this:
- (preferred) when
variant
is'fullscreen'
, automatically render<IconButton icon={XIcon} onClick={onClose} aria-label="Close" />
at the top right of the<AnchoredOverlay>
we use in<ActionMenu.Overlay>
- Update the Storybook example to render
<IconButton icon={XIcon} onClick={onClose} aria-label="Close" />
as a child of<ActionMenu.Overlay>
…menu-full-screen-story
👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks! |
Closes https://github.com/github/primer/issues/5155
This pull request introduces a new "fullscreen" variant for the
ActionMenu
component, along with a corresponding sample story to demonstrate its usage. The changes include updates to the component's props, default values, and example stories.Changelog
New
variant
prop available in ActionMenu.OverlayRollout strategy
Testing & Reviewing
Merge checklist