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

Update IconButtonKinds #18251

Closed

Conversation

albert-schilling
Copy link

@albert-schilling albert-schilling commented Dec 12, 2024

Add missing danger kinds

Closes #7211 , #11484

Refers to #14300, #14307, #17036

The IconButtonKinds were missing the danger options, even though the styles existed and the results are compelling.

I do not understand the reasoning behind why a destructive action should not be accessible via an IconButton, as mentioned in #4176. Any destructive action should require confirmation, such as through a modal. Using IconButtons helps reduce the visual load and keeps the UI slim and focused. Forcing designers or developers to use a regular button is not an ideal solution. Designers should have the flexibility to make this decision based on the specific context and the action's consequences.

If you agree with the suggested changes, I would be happy to update the Storybook story and the documentation in this PR as well.

Changelog

New

  • Adds danger, danger--tertiary, danger--ghost to the possible IconButtonKinds.
  • This change does not affect the runtime code but resolves a distracting error message caused by prop type validation.

Testing / Reviewing

  • Run storybook, visit the IconButton story, select e.g. danger as kind and check the console. There should be no prop type validation error.

Add missing danger and ghost kinds
@albert-schilling albert-schilling requested a review from a team as a code owner December 12, 2024 16:19
Copy link
Contributor

github-actions bot commented Dec 12, 2024

All contributors have signed the DCO.
Posted by the DCO Assistant Lite bot.

Copy link

netlify bot commented Dec 12, 2024

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

Name Link
🔨 Latest commit fb13ad9
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-web-components/deploys/675b0d17490f4a0008ab3b8b
😎 Deploy Preview https://deploy-preview-18251--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

netlify bot commented Dec 12, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit fb13ad9
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/675b0d175610f20008715447
😎 Deploy Preview https://deploy-preview-18251--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 Dec 12, 2024

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit fb13ad9
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/675b0d17f3d69c000849244a
😎 Deploy Preview https://deploy-preview-18251--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.

@albert-schilling
Copy link
Author

I have read the DCO document and I hereby sign the DCO.

@albert-schilling
Copy link
Author

recheck

@tay1orjones
Copy link
Member

@albert-schilling Hey, thanks for bringing this up! More detail on the justification behind the decision should be coming to the website via a new PR put up just after you opened this PR, carbon-design-system/carbon-website#4409. @alina-jacob might be able to share a preview of that content to help clarify.

So with the continued stance that this isn't recommended based on guidance from design,
I think updating the types like this would mislead devs and add to the confusion. You should be able to work around the error message by putting // @ts-expect-error on each usage or write your own wrapper component to use that suppresses the warning.

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.

Storybook emits prop type warning for tertiary and ghost danger buttons
2 participants