⚛️ Fix prop isShared
#3640
⚛️ Fix prop isShared
#3640
Conversation
| <OverflowMenuItem key={label}> | ||
| <Button component="a" {...btnProps}>{label}</Button> | ||
| </OverflowMenuItem> | ||
| ))} | ||
| </OverflowMenuGroup> | ||
| </OverflowMenuContent> | ||
| <OverflowMenuControl hasAdditionalOptions={overflow.some(i => !i.isShared)}> | ||
| <OverflowMenuControl hasAdditionalOptions={overflow.some(i => !i.shared)}> |
There was a problem hiding this comment.
Hm, it's a bit weird to negate a string 🤔
'cause !'false' == false, which might be confusing.
Why is this change done in the first place?...
There was a problem hiding this comment.
Hm, it's a bit weird to negate a string 🤔
'cause!'false' == false, which might be confusing.
It's supposed to be a boolean though.
On the other hand, HTML boolean attributes only care about its presence, not the actual value. That means these are the same:
<input disabled="false" />
<input disabled="true" />
Why is this change done in the first place?...
It's explained in the JIRA.isSharedlooks like a React props but it's passed to the HTML elements directly, so it should be namedisshared.
This attribute does nothing to the element really, we could remove it instead of passing it down. But IMHO it's more trouble than it's worth.
There was a problem hiding this comment.
I've been looking into this, and I have a question:
isSharedlooks like a React props but it's passed to the HTML elements directly, so it should be namedisshared.
But why do we need to pass it to HTML? 🤔
It looks like it's actually indeed a React (specifically Patternfly4 React) thing?...
https://v4-archive.patternfly.org/v4/components/overflow-menu#overflowmenudropdownitem
The naming is extremely confusing, by the way, "isShared" by whom?... So, if PF React really needs this, we probably shouldn't rename and should just avoid propagating this to HTML? 🤔
If in our implementation we don't need this React thing, we could also rename it to something more meaningful, like "alwaysHidden" defaulting to false (boolean)
There was a problem hiding this comment.
And by the way, I don't see the need to place the two buttons on the toolbar to the overflow menu... I would just keep them in actions. But well, that's fine.
There was a problem hiding this comment.
Oof, OK you're right, I completely lost sight of the real issue. isShared is passed both to Button and OverflowMenuDropdownItem but it's a prop of the latter and therefore an error is thrown for Button.
But the name can't change so I'm going to "remove" the attribute from btnProps so that it's not passed down to Button so that no error is thrown.
Such a good review @mayorova, thanks!
josemigallas
force-pushed
the
THREESCALE-10506_toolbar_shared
branch
from
c0639ea to
152dee4
Compare
THREESCALE-10506: TableToolbar: Rename isShared param to shared