Carbon: ModalWrapper: Properly unused prop `handleOpen`?

[IOIO72]

Summary

I'm not sure if I just don't see the use of the prop handleOpen.

The component uses a class method this.handleOpen, but the prop handleOpen seems not to be used in any way. I don't even see the prop passed down to a defined prop of the component Modal.

Maybe someone can have a look at this to check my supposition.

If I'm right, I would fix this issue together with a new feature I had in mind, when opening the source code.

Relevant information

Here's the line of the prop definition:
https://github.com/IBM/carbon-components-react/blob/0e2f293fc1731b1a848aae090a5d530e9deac369/src/components/ModalWrapper/ModalWrapper.js#L17

[emyarod]

hi @IOIO72 thanks for reporting this! yeah it looks like you're right that this prop is unused. I can go ahead and add it to the handleOpen class method on <ModalWrapper>, but if you would like to add more features to go along with it then I will defer to your PR

[IOIO72]

Hi @emyarod thanks for looking at this. I'll make my changes to the code today. I think I'll refactor the props of the component and add some descriptions, etc.

[IOIO72]

As suggested by @asudoh I post my question here.
Currently I'm refactoring the propType definitions of ModalWrapper and adding some descriptions. I've analysed the use of each prop and the pass thru of props to Modal.
In ModalWrapper I found some unused props inside the component itself and the passed thru props are not used in Modal as well: status handleOpen modalText withHeader and modalBeforeContent
And the prop onKeyDown in ModalWrapper seems to execute in both components, if defined - I think this could be a bug.
Some other props of Modal are explicitly defined in ModalWrapper and some are implicitly passed thru.

So, here's my question:

• should every definable prop of Modal be defined in ModalWrapper as well (duplicated), to allow a better documentation and a better check for used and unused props?
• Or should we just define the props used exclusively in ModalWrapper and never define props that are just passed thru to Modal? In the documentation we would just note, that all props are passed thru to Modal and those props are documented there ...

[asudoh]

Looking at the internal initial code - Seems that prop type definition of status handleOpen modalText withHeader and modalBeforeContent are redundant as @IOIO72 pointed out. Didn't see onKeyDown pass along to <Modal>. I think <Modal> props available to <ModalWrapper> being re-defined as prop types is a good idea.

BTW not sure if I have mentioned already - <ModalWrapper> merely is a demonstration-purpose component. We may want to deprecate it.

[stale[bot]]

We've marked this issue as stale because there hasn't been any activity for a couple of weeks. If there's no further activity on this issue in the next three days then we'll close it. Thanks for your contributions.

[tw15egan]

Is there any action we want to take on this issue?

[asudoh]

@tw15egan Just my thoughts; Probably not. New modal doc in the work that explains how to implement a state manager for modal (instead of using <ModalWrapper>) would be the ultimate solution.