-
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
Cut report last message to specified length #7124
Conversation
src/libs/actions/Report.js
Outdated
/** | ||
* Used before merging Report data. | ||
* Cut last message to specific length, because we don't need full last message | ||
* | ||
* @param {String} lastMessage | ||
* @returns {String} | ||
*/ | ||
function cutLastMessage(lastMessage) { | ||
return lastMessage.substr(0, CONST.REPORT.MAX_LAST_MESSAGE_LENGTH); | ||
} | ||
|
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.
Let's move this to reportUits as formatReportLastMessageText
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.
Done
src/libs/actions/Report.js
Outdated
// Alwasy cut last message | ||
updatedReportObject.lastMessageText = cutLastMessage(messageText); | ||
|
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.
Why did you move this out of if block
?
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.
There might be a chance that this if block is not executed, so last message is not being cut.
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.
Then it means that message is set somewhere else and we should cut the message there not here.
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.
I have add cuts to possible lastMessage
, so this is not necessary. Let's remove this.
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.
I see that it is important here. We can't remove it. So we can just move the line back to the previous location and add the formatting.
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.
Done
Co-authored-by: Rajat Parashar <[email protected]>
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.
Testing now...
Co-authored-by: Rajat Parashar <[email protected]>
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: @Julesssss
🎀 👀 🎀 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.
Tests well for me
✋ 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 staging by @Julesssss in version: 1.1.30-4 🚀
|
🚀 Deployed to production by @chiragsalian in version: 1.1.31-1 🚀
|
Details
Cut last message of a report to specific length, because we only use last message as alternate text on RHN and LHN.
Fixed Issues
$ #6699
Tests
QA Steps
Tested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android