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

HeaderMenu requiring isActive prop [Bug]: #15351

Closed
2 tasks done
dbryan17 opened this issue Dec 7, 2023 · 7 comments · Fixed by #15494
Closed
2 tasks done

HeaderMenu requiring isActive prop [Bug]: #15351

dbryan17 opened this issue Dec 7, 2023 · 7 comments · Fixed by #15494

Comments

@dbryan17
Copy link

dbryan17 commented Dec 7, 2023

Package

@carbon/react, @carbon/icons

Browser

Chrome

Package version

carbon/react 1.44.0, icons-react 11.31.0

React version

18

Description

For some reason the HeaderMenu component is requiring the isActive prop to always be true, or it breaks with:
Unhandled Runtime Error TypeError: Cannot read properties of undefined (reading 'isActive') Call Stack eval node_modules/@carbon/react/es/components/UIShell/HeaderMenu.js (151:70)
In earlier versions, I have never needed to provide the isActive prop to HeaderMenu.

Also, when following the carbon, react, next.js docs exactly (https://carbondesignsystem.com/developing/react-tutorial/step-1), I am getting the following warning in the console:
app-index.js:32 Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?
This is due to the use of the Link component around carbon components, as described in the docs.

The CodeSandbox below is currently broken due to another issue
It will not let you import UI Shell components due to the following error in the next icons package:
Failed to compile ./node_modules/@carbon/icons-react/es/generated/bucket-8.js Module parse failed: Export 'Infinity' is not defined (2620:1241) | WatsonHealthLaunchStudy_1.propTypes = iconPropTypes; | } export { IbmCloudPakIntegration, IbmCloudPakMantaAutomatedDataLineage, IbmCloudPakMulticloudMgmt,...
Described in the linked issue.

Once that issue is fixed, the CodeSandbox will show the errors described above.

Reproduction/example

https://codesandbox.io/p/devbox/headermenu-issue-88wg4c?file=%2Fapp%2Fnavbar.js%3A26%2C47

Steps to reproduce

run in dev, after installing dependencies

Suggested Severity

Severity 2 = User cannot complete task, and/or no workaround within the user experience of a given component.

Application/PAL

IBM CIO apps

Code of Conduct

@kuri-sun
Copy link
Contributor

kuri-sun commented Dec 9, 2023

Hi! I would love to take a look at this issue! Thanks!

@kuri-sun
Copy link
Contributor

kuri-sun commented Dec 13, 2023

Hi @guidari! I will create a PR for this after this issue is resolved! Thank you!

@guidari
Copy link
Contributor

guidari commented Dec 13, 2023

Hi @guidari! I will create a PR for this after this issue is resolved! Thank you!

Awesome! We have a PR open to fix it.
Thank you for your contributions! 🚀

@kuri-sun
Copy link
Contributor

Hello @dbryan17,
Could you try to reproduce this bug again with the new release, please?
I went to the sandbox that you provided but I could not get the error...
If you could get the error again, I would appreciate that if you could provide the sandbox link!
Thank you so much!! ✌️

@dbryan17
Copy link
Author

Hi @kuri-sun,
Thanks for looking into this. I've updated the sandbox and it now shows the error for this issue. https://codesandbox.io/p/devbox/headermenu-issue-88wg4c?file=%2Fapp%2Fnavbar.js%3A14%2C1

@kuri-sun
Copy link
Contributor

Hi @dbryan17! I saw the error right now! I will take a look at the issue and get you back ASAP! Thank you 😄

@kuri-sun
Copy link
Contributor

Hi, @dbryan17 and @guidari!
I found the cause of this problem and its solution, so I would like to share them here.

(Cause of the issue)
This happens because the hasActiveDescendant function does not consider the case when the HeaderMenu component has plain text as a child. The example that @dbryan17 provides us is using plain text as a child of the HeaderMenu component, so plain texts don't have any child.props, the function will throw an error.

const hasActiveDescendant = (childrenArg) =>
React.Children.toArray(childrenArg).some(
(child) =>
child.props.isActive ||
child.props.isCurrentPage ||
(child.props.children instanceof Array &&
hasActiveDescendant(child.props.children))
);

Screenshot 2023-12-29 at 11 01 55 AM
For example, If you put plain text inside the HeaderMenu component, you will see the error.

(Solution)
The solution for this issue is to make this component accept plain text elements and have the hasActiveDescendant function throw false when it is the case. So that I make sure that put a null check for child.props.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
3 participants