-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[HOLD for payment 2023-10-23] [$500] [Feature Request] Pressing back while writing notes makes text disappear #28599
Comments
ProposalPlease re-state the problem that we are trying to solve in this issue.
What is the root cause of that problem?
What changes do you think we should make in order to solve the problem?
Also, we can consider applying debounce to savePrivateNotesDraft What alternative solutions did you explore? (Optional)
|
Taking over this as an engineer(CME) as I worked on implementing the private notes feature so I will able to add some value here. |
ProposalPlease re-state the problem that we are trying to solve in this issue.Pressing back while writing notes makes text disappear. We should save drafts as people could be writing important stuff and go back/close modal by mistake. What is the root cause of that problem?The What changes do you think we should make in order to solve the problem?We need to pass App/src/pages/PrivateNotes/PrivateNotesEditPage.js Lines 118 to 131 in 76c1559
What alternative solutions did you explore? (Optional)xx |
Job added to Upwork: https://www.upwork.com/jobs/~012d86d9fa187b7292 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @robertKozik ( |
Quick question: What will be the behaviour if we edit multiple private notes? Would a single draft value handle all the edits or for each private notes we create unique draft to store it's value? |
As I mentioned in my proposal, for each private notes we should create a unique draft to store it`s value like what we did with draft messages |
@techievivek what do you think about the above? |
@DylanDylann OK, great, that sounds like a plan. Thanks |
@robertKozik can you please review the above proposals, thanks. |
ProposalPlease re-state the problem that we are trying to solve in this issue.
What is the root cause of that problem?
What changes do you think we should make in order to solve the problem?
function saveReportPrivateNoteDraft(reportID, note, accountID) {
// Save the draft private note to Onyx
// `reportPrivateNoteDraft_`
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_PRIVATE_NOTE_DRAFT}${reportID}`, note);
}
<TextInput
{...rest}
onChangeText={(text) => {
saveReportPrivateNoteDraft(reportID, text, session.accountID);
setNoteText(text);
}}
/>
const parser = new ExpensiMark();
+ const draftPrivateNot = useRef(getDraftPrivateNote(report.reportID, route.params.accountID));
const [privateNote, setPrivateNote] = useState(
- parser.htmlToMarkdown(lodashGet(report, ['privateNotes', route.params.accountID, 'note'], '')).trim()
+ draftPrivateNot.current || parser.htmlToMarkdown(lodashGet(report, ['privateNotes', route.params.accountID, 'note'], '')).trim()
); What alternative solutions did you explore? (Optional)
|
Hi @techievivek! I've checked the existing proposals, and the one from @DylanDylann looks good and it was the first one which described whole process of implementing this new feature, so I think we can go for it. As for the debounce suggestion, I'm all for it - we don't need excessive onyx calls. 🎀 👀 🎀 C+ reviewed |
Current assignee @techievivek is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new. |
Thanks for heads up @MariaHCD I've briefy checked our PR and I think as well that our PR reintroduced this problem. @DylanDylann It looks like this line is flawed https://github.com/Expensify/App/pull/29149/files#r1369162805 |
This issue has not been updated in over 15 days. @johncschuster, @techievivek, @robertKozik, @DylanDylann eroding to Monthly issue. P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do! |
Hey! I think the regression pointed before was fixed almost a month ago in this PR #30357. |
@johncschuster Could you help to process this issue? |
Sorry, I missed this! I'll process this later today! |
Ok, I'm back. @robertKozik I'm a bit confused – are you saying in this comment that the regression was already fixed? |
@johncschuster The regression fixed long time ago |
Not overdue, @johncschuster can you please take a look at the above comment. |
@johncschuster, @techievivek, @robertKozik, @DylanDylann Huh... This is 4 days overdue. Who can take care of this? |
Sorry for my mix-up, everyone! @DylanDylann, I've issued payment through Upwork. I've also invited @Someshwar-Tripathi to this payment job and will take care of that payment once it's been accepted. |
I've issued payment to @Someshwar-Tripathi. I think we can close this up! Please let me know if I've missed anything! |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
The note should not disappear
Actual Result:
The note disappears and we see a blank input page
Workaround:
Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number:
Reproducible in staging?:
Reproducible in production?:
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Screencast.from.19-09-23.01_16_24.AM.IST.webm
Expensify/Expensify Issue URL:
Issue reported by: @Someshwar-Tripathi
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695047398155859
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @johncschusterThe text was updated successfully, but these errors were encountered: