Ios conversation offline save changes button is disabled when opened to edit the comment for the second time #11822 #12426
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
pecanoro
requested review from
eVoloshchak and
dangrous
and removed request for
pecanoro
|
Hey @furkanuzundev (and @eVoloshchak), quick update! I'm getting one question about this issue/PR answered internally and then will be back with a review shortly - I just wanted to check in and let you know where I was. |
|
@dangrous Sure! I'm waiting. |
|
Okay I'm back! So here's the deal - given that I figured out an alternate way of dealing with this issue, though, that's much more contained. What if we edit this line in <OfflineWithFeedback
onClose={() => {
if (this.props.action.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) {
ReportActions.deleteOptimisticReportAction(this.props.report.reportID, this.props.action.clientID);
} else {
ReportActions.clearReportActionErrors(this.props.report.reportID, this.props.action.sequenceNumber);
}
}}
- pendingAction={this.props.action.pendingAction}
+ pendingAction={this.props.draftMessage ? null : this.props.action.pendingAction}
errors={this.props.action.errors}
errorRowStyles={[styles.ml10, styles.mr2]}
>That should be all we need! This will remove any visual effects on the edit section when it's open, and otherwise make no changes to the rest of the app (that I'm aware of). Let me know what you think (and if you can test, that would be amazing too)! If you both are in agreement, @furkanuzundev you can go ahead and update the PR with this change and we can move forward from there. |
|
@dangrous @eVoloshchak |
|
@dangrous, your change looks good to me as well, tested it working on iOS. And imo it is more self-explanatory, than having this prop inside the condition on @furkanuzundev, you can go ahead and update the PR |
|
updated |
|
|
@furkanuzundev, the code looks good and tests well, there are a couple of small issues with the issue description
|
|
@eVoloshchak |
|
@furkanuzundev I'm talking about the issue description, 'Screenshots" section. You've uploaded screenshots, but the testing steps can be best shown with a video, so please replace the screenshots with videos
Yes |
|
@eVoloshchak I have changed screenshots to video records that includes test steps. |
|
@dangrous, looks good to me too! |
|
This will be my first PR. We are witnessing a historical moment. :) |
|
Congrats, @furkanuzundev , I've merged it in! Great job getting your first Expensify/App pull request over the finish line! I know there's a lot of information in our contributing guidelines, so some points to take note of:
So it might take a while before you're paid for your work, but we typically post multiple new jobs every day, so there's plenty of opportunity. I hope you've had a positive experience contributing to this repo! |
|
@dangrous looks like this was merged without the checklist test passing. Please add a note explaining why this was done and remove the |
|
Not an emergency, all checks passed. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
Got it @dangrous.Thanks! |
|
🚀 Deployed to production by @yuwenmemon in version: 1.2.25-0 🚀
|
Details
The Save changes button is disabled when it was opened for the second time to edit the message.The screenshots in the below are includes i edit the message when i'm offline edit open the second time.
Fixed Issues
$ #11822
PROPOSAL: 11822(comment)
Tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesWaiting 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)PR Reviewer Checklist
The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** 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)Screenshots