LHN: Hide the pencil icon when message is sent or draft is deleted and show the icon when user started to type #6050 #6084
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
recheck |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
There was a problem hiding this comment.
@marktoman That's it for now, I really like this work, but we need to make sure no regressions come in. Great work so far. Can you please address the comments I left and we can give it another look once those are addressed. Thank you very much for taking this one on!
|
@mountiny Thanks for your suggestions, I have fixed all the issues. |
|
@marktoman Could you please update videos for all platforms and fill in the testing steps? Suggestion: Please mark this PR ready for review, if you think this is ready. A draft PR usually means you are still working. |
|
@parasharrajat I would rather let @mountiny confirm he has no changes in mind before marking it ready for merge. |
|
@marktoman Thank you very much for addressing my comments. In general, we have draft PRs only for when WIP and once ready for review (which this PR already was wirth the first iteration), contributor then makes the PR ready for review. Here is the dedicated part of Contributor guidelines. So @parasharrajat was correct, feel free to take this PR from draft state to "normal" state 😅 also beware you can trust @parasharrajat in terms of processes in this repository basically as much as any Expensify engineer :) Rajat also might be reviewing some of your next PRs before some Expensify engineer will get their hands on it. I will try to have a look into this soon so we can fix this asap :) |
|
@marktoman Additionally, to make this PR ready for review, you should fill in the appropriate sections of PR body. It is a bit tedious especially at the start to take screen recordings on all the platforms, but it is a process we require and which makes sure we test the change on all platforms. Also fill in the QA steps which our QA team could follow once this is deployed to staging. Thank you! |
There was a problem hiding this comment.
@marktoman That is it for me, great job on this not easy PR and considering this is your first job here, definitely not one of the easier to start with.
Do not hesitate to ask any questions :) I am here to help!
There was a problem hiding this comment.
Any updates here @marktoman, if you have any questions, do not hesitate to ask :)
MelvinBot
requested review from
mountiny
and removed request for
a team and
mountiny
|
@parasharrajat Status changed, my mistake. @mountiny Done. Thank you for the quick review on Saturday and apologies for missing some of the docs. I finally had time to read everything in detail over the weekend. I am not sure what to do about commit signing at this point? @parasharrajat @mountiny Video for each platform - sadly no luck after a lot of effort trying to run the app natively. The only thing that works is I have added steps and a video for both web and mobile web, capturing all LHN use cases. |
@marktoman Not sure what you mean by this. It appears to me that the commits from you are verified, which is good :) @marktoman First things first, please, pull main and merge the most recent code as it seems we have merge conflict there. I will review the code now. We are aware that there might be some problems developing on Windows. I am not totally knowledgeable in terms of how our contributors who do not own Mac do this, but surely there is many of them. Being you, feel free to ask in the Slack channel and someone will be able to help you/give you tips and advice as we require to test the PRs on all platforms usually. In this case we can safely assume the solution should behave same on mobile as on web as the change is mostly in the back logic. |
|
@mountiny Merged. OK, thanks for the advice. |
There was a problem hiding this comment.
@marktoman I tested it again and seems to me it works as expected. Thank you very much for including the detailed QA steps and the videos.
This is really great work and I think this should be my last changes requested. Really solid work here, Marek!
One comment for future regarding our PR body template. We use some automated processes based on webhooks to help us manage this repository and for that it is crucial to link the issue to PR which resolve it correctly and that is with the dollar sign and full URL. I noticed you have deleted the dollar sign from the template and left just the link. I have fixed it but just wanted to give you heads up for future PRs :) Thank you 🙇
The correct linking should look like: $ https://github.com/Expensify/App/issues/6050
|
@mountiny Alright, thank you for the guidance and overall handling of the issue! |
There was a problem hiding this comment.
@marktoman No, thank you :) I hope to see many more PRs from you 🎉 Waiting for the E2E tests before merging.
|
@mountiny Let's hope! Is the issue now considered finished so I can solve the other ones? |
|
@marktoman, 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 here are 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! 😊 |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
@marktoman As the handy comment above mentions, you can now be hired for other jobs :) But still one at a time, just to make sure you are able to update the PRs in timely manner (we expect addressing changes requested in PR reviews in ~2 days usually, if you can't update for whatever reason, let the engineer working on the issue with you know you will have delay) |
|
@marktoman a regression from this issue here: https://expensify.slack.com/archives/C01GTK53T8Q/p1636082766011900 After signing out, there is a blank page as SidebarLinks line 113 the unreadActionCount is undefined. Can you please look into this and try to fix it? Thank you |
|
Here's another related issue for that ^ #6228 (comment) |
|
Thanks @stitesExpensify! There is a PR up which will fix it, I will tag you for a review to speed it up! |
|
PR fixing this regression is up here. |
|
The accessibility issues found in this PR:
|
|
@ogumen Thank you very much for this feedback. For @marktoman, these issues are not regressions from this PR so no need to worry about that. I will create issue for the second issue mentioned here as well. |
|
Issue created here: #6386 |
Details
The pencil icon should not be visible when there is no draft in the report.
Show the pencil icon whenever users creates the draft (immediately) and hide the pencil icon as soon as the draft is either deleted or message sent (also immediately). However, keep the LHN reordering same, we do not want the LHN to reorder while typing as that is very distracting.
Fixed Issues
$ #6050
Tests and QA
Web, Desktop
Note: The pencil icon change is instant.
Actions
Action 1: Type text in the chat without sending it.
Result: Once you start typing, the pencil icon appears and remains visible on the contact in LHN. The order of the LHN items does not change.
Action 2: Type something in the chat and send it.
Result: The pencil icon appears as you type and disappears once you have sent it. The LHN order will change if not the topmost contact.
Action 3: Type text in the chat positioned in the middle of LHN. Click on another contact.
Result: The pencil icon remains visible while the LHN order changes.
Action 4: Have text in the chat and delete it.
Result: The pencil icon disappears while the LHN may re-order. All re-order cases are reproducible with other actions.
Action 5: Type text in the chat positioned in the middle of LHN. Reload the page.
Result: Contact appears at the top.
Action 6: (Depends on the previous step.) Delete the text for that contact.
Result: Contact appears in the middle again.
Action 7: Type text in the chat positioned in the middle of LHN. Receive a message from another contact positioned below the contact with a draft.
Result: LHN order updates.
Mobile Web, Android, iOS
Actions:
Repeat each desktop action but open LHN after each change.
Result: LHN is invisible when typing and always re-orders when visible.
Tested On
Screenshots
Web
Issue-6050-web.mp4
Mobile Web
Issue-6050-Mobile-1.mp4
Desktop
iOS
Android