[Bug/Enhancement] Pass PanelHeader props to the onClick handler of action items #1181
Conversation
| @@ -297,7 +297,7 @@ function PanelHeaderFactory(SaveExportDropdown, CloudStorageDropdown) { | |||
| if (item.dropdownComponent) { | |||
| showExportDropdown(item.id); | |||
There was a problem hiding this comment.
Should we ever have an action item that both shows a dropdown and does something on click? If not, then I can update this to return after this statement.
There was a problem hiding this comment.
My 2 cents: either-or is a reasonable assumption, weird to do both.
Signed-off-by: Wesam Manassra <[email protected]>
manassra
force-pushed
the
manassra/pass-props-to-action-item
branch
from
06a5fb0 to
28dddbd
Compare
| @@ -297,7 +297,7 @@ function PanelHeaderFactory(SaveExportDropdown, CloudStorageDropdown) { | |||
| if (item.dropdownComponent) { | |||
| showExportDropdown(item.id); | |||
| } | |||
| item.onClick(); | |||
| item.onClick && item.onClick(this.props); | |||
There was a problem hiding this comment.
Do we actually need to pass all the props, or can we just pick the one(s) we need? I generally favor more careful approach (eventually with companion types).
There was a problem hiding this comment.
How do we do that? The action can choose to call any action bound prop, which would be hard to predict here.
There was a problem hiding this comment.
yea for that you will need to add a getProp property to the item and have the items decide which prop to pass to onClick. performance wise, it's not going to matter. if just for the purpose of expose less props, maybe...but people would still need to know what are the props to choose from though, and they can just say give me all the props
There was a problem hiding this comment.
Exactly. With regards to getProp, they still have to derive it from all props, which would be the same problem as passing this.props to onClick.
| @@ -297,7 +297,7 @@ function PanelHeaderFactory(SaveExportDropdown, CloudStorageDropdown) { | |||
| if (item.dropdownComponent) { | |||
| showExportDropdown(item.id); | |||
There was a problem hiding this comment.
My 2 cents: either-or is a reasonable assumption, weird to do both.
| @@ -297,7 +297,7 @@ function PanelHeaderFactory(SaveExportDropdown, CloudStorageDropdown) { | |||
| if (item.dropdownComponent) { | |||
| showExportDropdown(item.id); | |||
| } | |||
| item.onClick(); | |||
| item.onClick && item.onClick(this.props); | |||
There was a problem hiding this comment.
Nit: Maybe init onClick to noop and drop the if?
| @@ -297,7 +297,7 @@ function PanelHeaderFactory(SaveExportDropdown, CloudStorageDropdown) { | |||
| if (item.dropdownComponent) { | |||
| showExportDropdown(item.id); | |||
| } | |||
| item.onClick(); | |||
| item.onClick && item.onClick(this.props); | |||
There was a problem hiding this comment.
yea for that you will need to add a getProp property to the item and have the items decide which prop to pass to onClick. performance wise, it's not going to matter. if just for the purpose of expose less props, maybe...but people would still need to know what are the props to choose from though, and they can just say give me all the props
Signed-off-by: Wesam Manassra <[email protected]>
PanelHeaderhas a prop calledactionItemswhich contains a list of actions, each with aid,tooltip,iconComponent,dropDownComponentand anonClickhandler:In order for the
onClickhandler to be able to dispatch actions, it needs access to thePanelHeader's prop, which this PR adds.