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

feat(menu): support menu modes #14793

Merged
merged 6 commits into from
Nov 27, 2023

Conversation

janhassel
Copy link
Member

Closes #14770

Adds the menu modes "basic" and "full" described in #14770 to support icons in MenuButton and ComboButton while restricting the use of nested and selectable items.

Opening as draft PR as the error throwing should be added to the unit tests, but first I wanted to clarify if throwing errors in this case is aligned to the Carbon ecosystem. A potential issue I see with this is that the errors are defined in MenuItem and therefore only throw once the MenuButton or ComboButton is opened.

Changelog

New

  • Added props.mode to Menu and its context
  • Throw errors when invalid Menu combinations are used

Testing / Reviewing

  • Manual testing for now (add MenuItemSelectable or MenuItemRadioGroup as children to MenuButton and ComboButton)
  • Automated testing tbd

@netlify
Copy link

netlify bot commented Oct 5, 2023

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 5bb62c5
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/655fad116eb8f80008a71755
😎 Deploy Preview https://deploy-preview-14793--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.

@netlify
Copy link

netlify bot commented Oct 5, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 53b2a3c
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/654beb50a2808f00083ddf8a
😎 Deploy Preview https://deploy-preview-14793--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.

@janhassel janhassel changed the title feat(menu): support menu mdoes feat(menu): support menu modes Oct 6, 2023
@tay1orjones
Copy link
Member

Thanks for submitting this @janhassel! Sorry for the delay in reviewing.

The biggest concern I have with throwing an error is that if this were to happen in production, React would completely unmount everything. Devs should be using ErrorBoundary to prevent this, but it's not guaranteed.

Instead of throwing an error, I think it might make more sense to pop a console.warn and scope that error to only when in a development environment. This would follow the same approach used for deprecations via the deprecate() helper. This relies on the internal warning helper that could be used directly instead of throwing the error. It has the built in invariant check for __DEV__ so these would only pop in non-production environments:

const warning = __DEV__
? function warning(condition, format, ...args) {
if (format === undefined) {
throw new Error(
'`warning(condition, format, ...args)` requires a warning ' +
'format argument'
);
}
if (!condition) {
let index = 0;
const message = format.replace(/%s/g, () => {
return args[index++];
});
console.warn('Warning: ' + message);
}
}
: emptyFunction;

@janhassel janhassel marked this pull request as ready for review November 3, 2023 09:37
@janhassel janhassel requested review from a team as code owners November 3, 2023 09:37
Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me!

Copy link
Contributor

@andreancardona andreancardona left a comment

Choose a reason for hiding this comment

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

apologies for the delay on this @janhassel I was OOO for a few days. Looks good 😎

@github-actions github-actions bot enabled auto-merge November 16, 2023 00:08
@github-actions github-actions bot added this pull request to the merge queue Nov 16, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 16, 2023
@janhassel
Copy link
Member Author

@andreancardona It looks like the auto-merge was cancelled. Could you take a look?

@tay1orjones tay1orjones added this pull request to the merge queue Nov 27, 2023
Merged via the queue into carbon-design-system:main with commit 18767e5 Nov 27, 2023
18 checks passed
@janhassel janhassel deleted the menu-modes branch November 28, 2023 08:13
danoro96 pushed a commit to danoro96/carbon that referenced this pull request Jan 18, 2024
* feat(menu): support menu mdoes

* refactor(menu): use warning instead of error for valid use

---------

Co-authored-by: Andrea N. Cardona <cardona.n.andrea@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Add decorative icon option to Menu
3 participants