-
Notifications
You must be signed in to change notification settings - Fork 3k
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 compose box resurfacing issue #9060
Changes from 16 commits
f1a598d
06386db
43fb53c
4a77995
ed8d0aa
f00048a
95e4723
46b7aa7
9d73a57
8cdd5e0
611e041
f5f94e8
50972f5
37aa68c
4d15590
f339b23
7669502
cff0bce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
import * as Session from '../actions/Session'; | ||
|
||
export default (shouldShowComposeInput, isSmallScreenWidth) => { | ||
export default (shouldShowComposeInput, isSmallScreenWidth = true) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking out loud: Thought about this lib name: This isn't really "toggling" anything, is it? It's just setting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I don't really understand this concern. Are you seeing a bug somewhere or something that needs to be addressed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope, no bug - this is related to my other 2 comments about web & desktop probably usually not being small screens, so it's weird to me that we default Again, not a blocker & doesn't need to be changed until we see a bug somewhere so can resolve this |
||
if (!isSmallScreenWidth) { | ||
return; | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -1,3 +1,4 @@ | ||||
import lodashGet from 'lodash/get'; | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NAB: Interested in using Ex:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not in this PR unless that code is being modified already. My usual preference is to only make the changes that are necessary. |
||||
import React from 'react'; | ||||
import {InteractionManager, View} from 'react-native'; | ||||
import PropTypes from 'prop-types'; | ||||
|
@@ -71,6 +72,8 @@ class ReportActionItemMessageEdit extends React.Component { | |||
this.triggerSaveOrCancel = this.triggerSaveOrCancel.bind(this); | ||||
this.onSelectionChange = this.onSelectionChange.bind(this); | ||||
this.addEmojiToTextBox = this.addEmojiToTextBox.bind(this); | ||||
this.saveButtonID = 'saveButton'; | ||||
this.cancelButtonID = 'cancelButton'; | ||||
|
||||
const parser = new ExpensiMark(); | ||||
const draftMessage = parser.htmlToMarkdown(this.props.draftMessage); | ||||
|
@@ -126,7 +129,6 @@ class ReportActionItemMessageEdit extends React.Component { | |||
* allows one to navigate somewhere else and come back to the comment and still have it in edit mode. | ||||
* @param {String} newDraft | ||||
*/ | ||||
|
||||
debouncedSaveDraft(newDraft) { | ||||
Report.saveReportActionDraft(this.props.reportID, this.props.action.reportActionID, newDraft); | ||||
} | ||||
|
@@ -210,6 +212,14 @@ class ReportActionItemMessageEdit extends React.Component { | |||
ReportScrollManager.scrollToIndex({animated: true, index: this.props.index}, true); | ||||
toggleReportActionComposeView(false, VirtualKeyboard.shouldAssumeIsOpen()); | ||||
}} | ||||
onBlur={(event) => { | ||||
// Return to prevent re-render when save/cancel button is pressed which cancels the onPress event by re-rendering | ||||
if (_.contains([this.saveButtonID, this.cancelButtonID], lodashGet(event, 'nativeEvent.relatedTarget.id'))) { | ||||
return; | ||||
} | ||||
|
||||
toggleReportActionComposeView(true, VirtualKeyboard.shouldAssumeIsOpen()); | ||||
}} | ||||
Comment on lines
+215
to
+222
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @marcaaron Could you please help me understand this change? It has caused an issue #9252 and I am trying to figure out what is it doing? Especially What do I need to test for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are you curious about exactly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Want to maybe walk me through what you think it's doing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So it is checking that if the Cancel or save button is clicked on the Editor, return, otherwise show the composer. So do we want to show the composer on blur? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIRC, in this case we are ignoring the blur event because when we are blurring because the cancel or save button is clicked the method to toggle the composer ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got, It. Thanks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to show the composer on blur? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes on blur and it should happen when we save changes or cancel the edit right? Can we have this conversation on whatever issue you are working on so I can get more insight into what you're trying to figure out?
This comment was marked as off-topic.
Sorry, something went wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please bring whatever conversation you'd like to have to a relevant issue. |
||||
selection={this.state.selection} | ||||
onSelectionChange={this.onSelectionChange} | ||||
/> | ||||
|
@@ -226,12 +236,14 @@ class ReportActionItemMessageEdit extends React.Component { | |||
<Button | ||||
small | ||||
style={[styles.mr2]} | ||||
nativeID={this.cancelButtonID} | ||||
onPress={this.deleteDraft} | ||||
text={this.props.translate('common.cancel')} | ||||
/> | ||||
<Button | ||||
small | ||||
success | ||||
nativeID={this.saveButtonID} | ||||
style={[styles.mr2]} | ||||
onPress={this.publishDraft} | ||||
text={this.props.translate('common.saveChanges')} | ||||
|
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.
NAB; If
.replaceAll
is a native JS function (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replaceAll) why do we prefer using a lib for it here?Also,
.replace(/ /g, '')
is basically another.replaceAll(message, ' ', '')
so why not replace that one too?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.
it's poly-filled and not available on some mobile browsers
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.
Cool ya makes sense
Any thoughts on my second comment?
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.
Mostly added this to fix a bad crash with iOS 14 so that wasn't my priority. We could change the usages, but that doesn't stop people from making this mistake in the future. I also added an eslint rule to prevent people from using
string.prototype.replaceAll
.