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
fix missing dialog props (such as onShow) #1715
Conversation
src/components/dialog/index.tsx
Outdated
@@ -247,7 +247,7 @@ class Dialog extends Component<DialogProps, DialogState> { | |||
|
|||
render = () => { | |||
const {modalVisibility} = this.state; | |||
const {testID, supportedOrientations, accessibilityLabel, ignoreBackgroundPress} = this.props; | |||
const {testID, supportedOrientations, accessibilityLabel, ignoreBackgroundPress, ...others} = this.props; |
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.
This should be done with modalProps
:
/**
* Additional props for the modal.
*/
modalProps?: ModalProps;
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.
@M-i-k-e-l
We are currently declaring that Dialog props are extending Modal props. (code wise, we don't do it)
If you want to go with adding modalProps
which I agree is safer, we should fix the component typings as well
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.
We have a Pick
from ModalPropsIOS
, or did I miss something? Generally we should remove that if we're adding the suggested prop, but I don't think it's worth the migration since we're going to move to the new Dialog
soonish.
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.
Fixed :)
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 run npm run build:dev
🙏
And it has conflicts :/
Fixed the conflict. |
Description
Fix missing spread props
pickerModalProps
passed toPicker
Changelog
Fix missing
pickerModalProps
props spread passed toPicker