Correct some improper code patterns #8733
Conversation
| }, copyShortcutConfig.descriptionKey, copyShortcutConfig.modifiers, false); | ||
| this.unsubscribeCopyShortcut = KeyboardShortcut.subscribe( | ||
| copyShortcutConfig.shortcutKey, | ||
| this.copySelectionToClipboard, |
There was a problem hiding this comment.
question: Do we need to bind this in the constructor since we are refactoring the arrow function?
There was a problem hiding this comment.
Doesn't look like the function is referencing this, so it looks like we don't need to in this case.
There was a problem hiding this comment.
separate thought: Why do we need a keyboard event for copying text? 😵💫
Maybe we can add some context to the copySelectionToClipboard() JSDoc.
There was a problem hiding this comment.
Yeah, Rory is correct.
As for why it's needed, it came from this PR #6732 and it's so that markdown formatting is preserved when copying selected text.
Which indicates that I need to update the testing instructions 😁
| this.setState({localUnreadActionCount: 0}); | ||
| this.setState({ | ||
| isMarkerActive: false, | ||
| localUnreadActionCount: 0, |
There was a problem hiding this comment.
@parasharrajat @roryabraham do either of you know whether this callback is doing anything?
introduced here: #4773
There was a problem hiding this comment.
I think I moved the count reset to the callback so that the user does not see an immediate reset to 0 while the MarkerBadge is hiding. Not the best way to do this as animation has a timeout but this callback will fire early.
There was a problem hiding this comment.
so that the user does not see an immediate reset to 0 while the MarkerBadge is hiding. Not the best way to do this as animation has a timeout but this callback will fire early.
thought: Perhaps a better way to do that is to wait until the animation is over to set the unread action count.
@tgolen not sure if we need to do anything here though as unclear if the original concern is something we should solve for i.e.
so that the user does not see an immediate reset to 0 while the MarkerBadge is hiding
There was a problem hiding this comment.
IMO since the animation is short, it doesn't really matter what the count is. I agree that if it does matter, listening to the end of the animation event sounds like a better solution.
|
Bump for reviews again. I don't think either of the comments above need to be addressed with changes. |
|
🚀 Cherry-picked to staging by @sketchydroide in version: 1.1.57-8 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
|
Btw, QA is unable to test part of this because of this issue. |
|
OK, yeah, I think checking it off is fine. |
|
🚀 Deployed to production by @chiragsalian in version: 1.1.57-17 🚀
|
|
Coming from #8930 |
Details
As I was going through the code debugging another issue, I ran into these bad patterns.
Tests and QA
Test the comment copy
Test the unread message marker
Screenshots
Web
Mobile Web
Desktop
iO
Android