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

docs(modal-wrapper): remove invalid prop, fix renderTriggerButtonIcon knob #4830

Merged
merged 2 commits into from
Dec 6, 2019
Merged

docs(modal-wrapper): remove invalid prop, fix renderTriggerButtonIcon knob #4830

merged 2 commits into from
Dec 6, 2019

Conversation

jendowns
Copy link
Contributor

@jendowns jendowns commented Dec 6, 2019

The ModalWrapper demo in the React storybook environment has an invalid prop triggerButtonIcon being passed to it.

Screen Shot 2019-12-06 at 2 22 17 PM

Error text:

Warning: React does not recognize the `triggerButtonIcon` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `triggerbuttonicon` instead. If you accidentally passed it from a parent component, remove it from the DOM element.

This demo also has a default value of 'none' for the renderTriggerButtonIcon on page load, which is an invalid value & generates a console error in local development. (The error goes away when you use knobs to select an icon, because then the value being passed changes from 'none' to a valid icon).

Screen Shot 2019-12-06 at 2 22 09 PM

Text of two console errors on page load:

Warning: Failed prop type: Invalid prop `renderTriggerButtonIcon` supplied to `ModalWrapper`.
    in ModalWrapper (created by Container)
    in Container (created by storyFn)
    in storyFn
    in ErrorBoundary

Warning: Failed prop type: Invalid prop `renderIcon` supplied to `Button`.
    in Button (created by ModalWrapper)
    in ModalWrapper (created by Container)
    in div (created by Story)
    in Story (created by Container)
    in div (created by Container)
    in StrictMode (created by Container)
    in Container (created by storyFn)
    in storyFn
    in ErrorBoundary

This PR proposes removing the invalid prop triggerButtonIcon and also updating the renderTriggerButtonIcon prop's knob logic so that, on page load, renderTriggerButtonIcon={undefined} (valid) and not renderTriggerButtonIco='none' (invalid)

Changelog

Changed

  • change default value for renderTriggerButtonIcon prop knob. Now it will be renderTriggerButtonIcon={undefined} (valid) on page load, and not renderTriggerButtonIco='none' (invalid)

Removed

  • remove triggerButtonIcon (not a valid prop)

Testing / Reviewing

I suggest running the storybook locally, and viewing the console in Chrome.

@jendowns jendowns requested a review from a team as a code owner December 6, 2019 20:25
@ghost ghost requested review from asudoh and dakahn December 6, 2019 20:25
renderTriggerButtonIcon: typeof iconToUse === 'function' && iconToUse,
triggerButtonIcon: typeof iconToUse !== 'function' && iconToUse,
renderTriggerButtonIcon:
typeof iconToUse === 'function' ? iconToUse : undefined,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quick note: the line break + indentation is prettier formatting from the pre-commit hook 😅

@netlify
Copy link

netlify bot commented Dec 6, 2019

Deploy preview for the-carbon-components ready!

Built with commit e2099cf

https://deploy-preview-4830--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Dec 6, 2019

Deploy preview for carbon-components-react ready!

Built with commit e2099cf

https://deploy-preview-4830--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Dec 6, 2019

Deploy preview for carbon-elements ready!

Built with commit e2099cf

https://deploy-preview-4830--carbon-elements.netlify.com

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - Thanks @jendowns!

@asudoh asudoh merged commit bf14fb7 into carbon-design-system:master Dec 6, 2019
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.

None yet

3 participants