-
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
Update isReportMessageAttachment logic #8117
Conversation
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.
Do you see a pattern here that can be optimized?
src/libs/reportUtils.js
Outdated
function isReportMessageAttachment(reportMessageText, reportMessageHtml) { | ||
return reportMessageText === '[Attachment]' && reportMessageHtml !== '[Attachment]'; |
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.
could put the lodashGet
in here rather than passing it in as an argument.
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.
Whoops! Sorry @parasharrajat, didn't see this before leaving this 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.
No worries.
So, they are extracted from variable( |
Ok. Awesome. let's do it. pass a single param to that function that checks both values. |
Is this what you are suggesting?
|
This comment was marked as resolved.
This comment was marked as resolved.
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.
LGTM.
cc: @Luke9389
PR Reviewer Checklist
- I verified the PR has a small number of commits behind
main
- I verified the correct issue is linked in the
### Fixed Issues
section above - I verified testing steps are clear and they cover the changes made in this PR
- I verified the testing environment is mentioned in the test steps
- I verified testing steps cover success & fail scenarios (if applicable)
- I checked that screenshots or videos are included for tests on all platforms
- I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
- I verified there are no console errors related to changes in this PR
- I verified proper code patterns were followed (see Reviewing the code)
- I verified comments were added when the code was not self explanatory
- I verified any copy / text shown in the product was added in all
src/languages/*
files (if applicable) - I verified proper naming convention for platform-specific files was followed (if applicable)
- I verified style guidelines were followed
- I verified the JSDocs style guidelines (in
STYLE.md
) were followed
- I verified that this PR follows the guidelines as stated in the Review Guidelines
- I verified other components are not impacted by changes in this PR (i.e. if the PR modifies a shared library or component like
Avatar
, I verified the components usingAvatar
are working as expected) - I verified the UI performance was not affected (the performance is the same than
main
branch) - If a new component is created I verified that a similar component doesn't exist in the codebase
🎀 👀 🎀 C+ reviewed
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.
Lookin' good.
Just one NAB. Up to you if you'd like to implement that.
Co-authored-by: Luke Donahue <49007721+Luke9389@users.noreply.github.com>
@Luke9389 Made the suggested change. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to production by @roryabraham in version: 1.1.51-0 🚀
|
🚀 Deployed to production by @roryabraham in version: 1.1.51-0 🚀
|
Details
Solves the issue "When [Attachment] is sent as the message, 'Edit Comment' option does not appear in contextMenu "
Fixed Issues
$ #7739
Tests
PR Review Checklist
Contributor (PR Author) Checklist
main
### Fixed Issues
section abovesrc/languages/*
files (if applicable)Styling.md
) for all style edits I madeSTYLE.md
)Avatar
, I verified the components usingAvatar
are working as expected)main
branch)PR Reviewer Checklist
main
### Fixed Issues
section abovesrc/languages/*
files (if applicable)STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)main
branch)QA Steps
Screenshots
Web
Screen.Recording.2022-03-13.at.21.18.36.mov
Mobile Web
Screen.Recording.2022-03-13.at.22.47.22.mov
Desktop
Screen.Recording.2022-03-13.at.21.34.43.mov
iOS
Screen.Recording.2022-03-13.at.22.45.17.mov
Android
Screen.Recording.2022-03-13.at.23.19.19.mov