-
Notifications
You must be signed in to change notification settings - Fork 957
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: add change orientation event with modal render #1346
Conversation
@@ -93,7 +94,15 @@ export class Modal extends React.PureComponent<ModalProps, State> { | |||
this.modalId = ModalService.hide(this.modalId); | |||
}; | |||
|
|||
public forceRender = (): void => { |
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.
public forceRender = (): void => { | |
private forceRender = (): void => { |
Should not be public, should it?
If not, it should be declared below lifecycle methods
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.
forceRender
should not be public most likely. I would suggest making it private and declaring it below the lifecycle methods then. If we leave it public, it's better to call that forceUpdate
or simply update
.
Will it work correctly if the modal will contain some state? For example, if the user will put there |
Sure thing, it works fine with any children component inside. I've updated the method and made it private. Take a look |
Please read and mark the following check list before creating a pull request:
Short description of what this resolves:
The solutions is quite simple - I just render new modal with new id. I dont think there would be a possibility to apply new position to old one. If you think we can apply new position for the modal - let me know pls
Closes #1132