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

fix(HeaderPanel): Initialize onHeaderPanelFocus with default value #14351

Merged
merged 7 commits into from
Aug 4, 2023
Merged

Conversation

sjbeatle
Copy link
Contributor

Changelog

Changed

  • HeaderPanel has a prop, onHeaderPanelFocus, which is typed to be an optional function, but the component logic calls the function indiscriminately as if it were a required property. This is causing adopters to have errors when using HeaderPanel without that property defined.

Updating the component to initialize the optional prop with an inert function. I'm not sure if that's an acceptable fix, or not. Alternatives may be:

  • Make onHeaderPanelFocus required
  • Check for onHeaderPanelFocus being defined prior to calling it within the component's logic.

Testing / Reviewing

Specs have been added. Checkout this branch and run yarn test -- headerpanel with and without the onHeaderPanelFocus prop being initialized to see the fix.

    onHeaderPanelFocus = () => {},

vs.

    onHeaderPanelFocus,

@sjbeatle sjbeatle requested a review from a team as a code owner July 31, 2023 15:58
@netlify
Copy link

netlify bot commented Jul 31, 2023

Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit d367f50
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/64cd19ce2b18c20008058b0b
😎 Deploy Preview https://deploy-preview-14351--carbon-components-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 Jul 31, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit d367f50
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/64cd19ce92156a00092389ab
😎 Deploy Preview https://deploy-preview-14351--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
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.

Looks great, thanks for this! Just a couple minor suggestions

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2023

DCO Assistant Lite bot: Thanks for your submission! We ask that you all sign our Developer Certificate of Origin before we can accept your contribution. You can sign the DCO by adding a comment below using this text:


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


2 out of 3 committers have signed the DCO.
@sjbeatle
@tay1orjones
@steven Black
Steven Black seems not to be a GitHub user. You need a GitHub account to be able to sign the DCO. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request

@tay1orjones
Copy link
Member

I don't think there's a way to override the DCO check specifically but we can merge with that one failing here I think.

@sjbeatle
Copy link
Contributor Author

sjbeatle commented Aug 4, 2023

I don't think there's a way to override the DCO check specifically but we can merge with that one failing here I think.

Thanks @tay1orjones!!

Thank you for pushing those changes, as well. I made the noopFn update.... I think that was all. Please let me know if there's anything else you need from me for this PR. 😁

@tw15egan tw15egan added this pull request to the merge queue Aug 4, 2023
Merged via the queue into carbon-design-system:main with commit ab551c2 Aug 4, 2023
16 of 17 checks passed
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