-
Notifications
You must be signed in to change notification settings - Fork 99
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
Menu permissions #1207
Menu permissions #1207
Conversation
Codecov Report
@@ Coverage Diff @@
## gsa-8.0 #1207 +/- ##
==========================================
- Coverage 16.51% 16.5% -0.02%
==========================================
Files 948 948
Lines 27592 27610 +18
Branches 5906 5925 +19
==========================================
Hits 4558 4558
- Misses 21580 21594 +14
- Partials 1454 1458 +4
Continue to review full report at Codecov.
|
gsa/src/web/components/menu/menu.js
Outdated
@@ -114,13 +116,24 @@ const MenuList = styled.ul` | |||
} | |||
`; | |||
|
|||
const checkChild = child => { | |||
if (child.type === MenuSection && isArray(child.props.children)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not 100 percent sure if this will work. React has a special API to access children because they aren't arrays https://reactjs.org/docs/react-api.html#reactchildren
gsa/src/web/components/menu/menu.js
Outdated
@@ -114,13 +116,24 @@ const MenuList = styled.ul` | |||
} | |||
`; | |||
|
|||
const checkChild = child => { | |||
if (child.type === MenuSection && isArray(child.props.children)) { | |||
return child.props.children.find(chil => chil !== false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am really sure this won't work always. Please use the React.Children API (https://reactjs.org/docs/react-api.html#reactchildren) and I guess null and undefined should be checked too. Maybe use
chil => !!chil
gsa/src/web/components/menu/menu.js
Outdated
if (isDefined(to)) { | ||
link = <Link to={to}>{title}</Link>; | ||
} else if (isDefined(children) && children.length > 0) { | ||
const [child] = children; | ||
let [child] = children; | ||
child = checkChild(child); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO the function name checkChild is a bit misplaced. I do expect either to return a boolean or to raise an exception for a check named function. I guess
getFirstMenuEntry(children)
would fit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at the children handling if that really fits
If the first child is a section, the DefaultEntry receives that section as title element instead of the correct menu title.
e1b9f18
to
a5180bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This fixes wrongly displayed menues. We need to make sure that the first child of a Menu is a MenuEntry and not a MenuSection.
Checklist: