replace onModalHide with onDismiss for the web #15298
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi, @mollfpr @cristipaval , I'm not quit sure if this replacement approach is in line with specification and would appreciate if you could share your thoughts. 😄 |
melvin-bot
bot
requested review from
cristipaval and
mollfpr
and removed request for
a team
|
@cristipaval @mollfpr One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@ntdiary How about adding new props cc @cristipaval |
The main diffs are as follows, I think we might also need to call `onDismiss` in `componentWillUnmount` to ensure compatibility?diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js
index dc29bc4896..904dd43036 100644
--- a/src/components/Modal/BaseModal.js
+++ b/src/components/Modal/BaseModal.js
@@ -16,11 +16,15 @@ const propTypes = {
/** The ref to the modal container */
forwardedRef: PropTypes.func,
+
+ /** Ensure that callback and trap deactivation are in the same loop on the web platform */
+ onDismiss: PropTypes.func,
};
const defaultProps = {
...modalDefaultProps,
forwardedRef: () => {},
+ onDismiss: () => {},
};
class BaseModal extends PureComponent {
@@ -28,6 +32,7 @@ class BaseModal extends PureComponent {
super(props);
this.hideModal = this.hideModal.bind(this);
+ this.onDismiss = this.onDismiss.bind(this);
}
componentDidUpdate(prevProps) {
@@ -41,6 +46,14 @@ class BaseModal extends PureComponent {
componentWillUnmount() {
// we don't want to call the onModalHide on unmount
this.hideModal(this.props.isVisible);
+ this.onDismiss(this.props.isVisible);
+ }
+
+ onDismiss(callHideCallback = true) {
+ if (!callHideCallback) {
+ return;
+ }
+ this.props.onDismiss();
}
/**
@@ -99,6 +112,7 @@ class BaseModal extends PureComponent {
}}
propagateSwipe={this.props.propagateSwipe}
onModalHide={this.hideModal}
+ onDismiss={this.onDismiss}
onSwipeComplete={this.props.onClose}
swipeDirection={swipeDirection}
isVisible={this.props.isVisible}
diff --git a/src/components/Modal/index.web.js b/src/components/Modal/index.web.js
index 3cb6f3561b..980d978621 100644
--- a/src/components/Modal/index.web.js
+++ b/src/components/Modal/index.web.js
@@ -29,7 +29,8 @@ const Modal = (props) => {
<BaseModal
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
- onModalHide={hideModal}
+ onDismiss={hideModal}
+ onModalHide={() => {}}
onModalShow={showModal}
>
{props.children} |
|
Suggestion: I think we should use |
|
If we are also considering diff --git a/src/components/Modal/index.web.js b/src/components/Modal/index.web.js
index 980d978621..eddcdd39b1 100644
--- a/src/components/Modal/index.web.js
+++ b/src/components/Modal/index.web.js
@@ -29,8 +29,8 @@ const Modal = (props) => {
<BaseModal
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
- onDismiss={hideModal}
- onModalHide={() => {}}
+ onDismiss={props.fullscreen ? hideModal : () => {}}
+ onModalHide={!props.fullscreen ? hideModal() : () => {}}
onModalShow={showModal}Perhaps the first revision would be more concise and easier to understand? diff:diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js
index dc29bc4896..fbbe10c6b3 100644
--- a/src/components/Modal/BaseModal.js
+++ b/src/components/Modal/BaseModal.js
@@ -16,11 +16,15 @@ const propTypes = {
/** The ref to the modal container */
forwardedRef: PropTypes.func,
+
+ /** Ensure that callback and trap deactivation are in the same loop on the web platform */
+ useOnDismiss: PropTypes.bool,
};
const defaultProps = {
...modalDefaultProps,
forwardedRef: () => {},
+ useOnDismiss: false,
};
class BaseModal extends PureComponent {
@@ -98,7 +102,8 @@ class BaseModal extends PureComponent {
this.props.onModalShow();
}}
propagateSwipe={this.props.propagateSwipe}
- onModalHide={this.hideModal}
+ onDismiss={this.props.useOnDismiss ? this.hideModal : () => {}}
+ onModalHide={!this.props.useOnDismiss ? this.hideModal : () => {}}
onSwipeComplete={this.props.onClose}
swipeDirection={swipeDirection}
isVisible={this.props.isVisible}
diff --git a/src/components/Modal/index.web.js b/src/components/Modal/index.web.js
index 3cb6f3561b..e61225e1e2 100644
--- a/src/components/Modal/index.web.js
+++ b/src/components/Modal/index.web.js
@@ -29,6 +29,7 @@ const Modal = (props) => {
<BaseModal
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
+ useOnDismiss={props.fullscreen}
onModalHide={hideModal}
onModalShow={showModal}
> |
|
Hi, @mollfpr @cristipaval , could we move on with the latest version?
Personally, I think the former is easy to understand and maintain, but will check conditions for all platforms. Please let me know if there are any concerns. 😄 |
|
@ntdiary I'm genuinely sorry for the delay. Could you point out which is the former one? Is it the original one? |
@mollfpr, yeh 😄, the main diffs: diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js
index dc29bc4896..fbbe10c6b3 100644
--- a/src/components/Modal/BaseModal.js
+++ b/src/components/Modal/BaseModal.js
@@ -16,11 +16,15 @@ const propTypes = {
/** The ref to the modal container */
forwardedRef: PropTypes.func,
+
+ /** Ensure that callback and trap deactivation are in the same loop on the web platform */
+ useOnDismiss: PropTypes.bool,
};
const defaultProps = {
...modalDefaultProps,
forwardedRef: () => {},
+ useOnDismiss: false,
};
class BaseModal extends PureComponent {
@@ -98,7 +102,8 @@ class BaseModal extends PureComponent {
this.props.onModalShow();
}}
propagateSwipe={this.props.propagateSwipe}
- onModalHide={this.hideModal}
+ onDismiss={this.props.useOnDismiss ? this.hideModal : () => {}}
+ onModalHide={!this.props.useOnDismiss ? this.hideModal : () => {}}
onSwipeComplete={this.props.onClose}
swipeDirection={swipeDirection}
isVisible={this.props.isVisible}
diff --git a/src/components/Modal/index.web.js b/src/components/Modal/index.web.js
index 3cb6f3561b..e61225e1e2 100644
--- a/src/components/Modal/index.web.js
+++ b/src/components/Modal/index.web.js
@@ -29,6 +29,7 @@ const Modal = (props) => {
<BaseModal
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
+ useOnDismiss={props.fullscreen}
onModalHide={hideModal}
onModalShow={showModal}
> |
|
@ntdiary Yeah, that seems maintainable and makes sense to me. What do you think @cristipaval? |
|
@cristipaval hi, is there anything else I can do for this pr? 😄 |
|
Sorry, I'll review it within the next few hours. |
cristipaval
reviewed
Feb 23, 2023
There was a problem hiding this comment.
Changes look good to me, the thing is that I cannot reproduce the issue on main branch either on the physical iOS device or on the simulator. @mollfpr Can you reproduce it with the latest main?
|
@cristipaval Sorry I can’t confirm it, I don’t have my laptop now but I can reproduce it on the staging and production with my device. @ntdiary Can you confirm the @cristipaval comment? |
|
@mollfpr @cristipaval yeh, I can reproduce it with the latest main on the simulator (I don't have a physical IOS device now) simulator.mp4 |
Reviewer Checklist
Screenshots/VideosWeb15298.Web.movMobile Web - Chrome15298.mWeb.Chrome.mp4Mobile Web - Safari15298.mWeb.Safari.mp4Desktop15298.Desktop.moviOS15298.iOS.mp4Android15298.Android.mp4 |
|
@ntdiary I see your iOS video attach that the composer keeps showing after closing the emojis modal, but it is not happening to me. Can you confirm it? |
|
@mollfpr , oh, sorry. I didn't enable the soft keyboard when I recorded the old video, have updated it. 😅 |
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
github-actions
bot
added
the
DeployBlockerCash
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
|
Hi, @mollfpr @cristipaval , is there anything we need to do for this performance report? |
55 tasks
This was referenced Feb 24, 2023
|
just to give everyone a little heads up here, this PR was reverted b/c we thought it was the root cause of the performance regression, that PR was #15495. I created #15498 to revert the revert PR to get the code back into main for you, looks like it auto assigned Cristi and Luthfi to review again since they are straight reverts we should be fine to just merge so you can copy/paste the reviewer checklist from here into #15498 if you want. Sorry for all the confusion. |
29 tasks
|
🚀 Deployed to staging by https://github.com/cristipaval in version: 1.2.77-0 🚀
|
|
🚀 Deployed to staging by https://github.com/cristipaval in version: 1.2.77-0 🚀
|
Closed
1 task
|
@ntdiary the PR introduced this regression and we reverted |
6 tasks
|
I am really lost with this PR. |
|
@mvtglobally The first revert and revert the revert are not related to this regression. We though this PR caused a performance regression but apparently not. The last revert is related to the bug here. I'm not sure what you are testing here, but I guess you need to perform the test steps mentioned here #15559 |
This comment was marked as outdated.
This comment was marked as outdated.
|
@ntdiary Thank you for your investigation. Could you possibly post this in the issue, so that we have all technical discussions in one place, together with the proposals? |
|
@cristipaval, sure, have done. 😄 |
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.2.77-4 🚀
|
This was referenced Mar 2, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Fixed Issues
$ #14848
PROPOSAL: #14848 (comment)
Tests
Offline tests
N/A
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
web.mp4
Mobile Web - Chrome
android-chrome.mp4
Mobile Web - Safari
ios-safari.mp4
Desktop
desktop.mp4
iOS
ios-app.mp4
Android
android-app.mp4