-
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
Account for markdown using debounced comment counter #15501
Conversation
@francoisl @aimane-chnaif One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Code looks good to me and tests well on all various cases. |
@aimane-chnaif Thank you. I am not certain that |
When html length reaches threshold: max.html.length.movWhen text length reaches threshold: max.text.length.movFor user experience, it's a bit odd when exceed on smaller length, not exceed on larger length. |
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chromemchrome.movMobile Web - Safarimsafari.mp4Desktopdesktop.moviOSios.mp4Androidandroid.mov |
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 and tests well 🎉
All yours @francoisl
@aimane-chnaif I did notice that case as well. When you append text to the last Since this is how the back-end would see the length as well, this is logical, however I agree it should be documented to not raise an issue later. I am unsure if a unit test would be the right place. Perhaps it could be documented in test rail after we define the regression steps. I will defer to @francoisl. |
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.
Works well, thanks!
@@ -402,6 +402,7 @@ const CONST = { | |||
SHOW_LOADING_SPINNER_DEBOUNCE_TIME: 250, | |||
TOOLTIP_SENSE: 1000, | |||
TRIE_INITIALIZATION: 'trie_initialization', | |||
COMMENT_LENGTH_DEBOUNCE_TIME: 500, |
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.
Usually I like to include the unit for time constants (e.g. append _MS
) to make it clear, but we already don't do it for the other constants above, so not blocking on this.
import PropTypes from 'prop-types'; | ||
import {debounce} from 'lodash'; |
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.
Interesting, in the rest of the codebase I see that we use UnderscoreJS's debounce
function, not the one from lodash. AFAICT they must be pretty much the same though.
Congrats, that’s your 5th PR merged! 🎉 Do you know about the ContributorPlus role? It’s an opportunity to earn more in the Expensify Open Source community. Keep up the great work - thanks! |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
@francoisl @aimane-chnaif This looks like it is due to the calculation occurring when the page is loaded, which could |
@redstar504 I don't think this PR is related to this performance regression. This is now happening to all PRs recently. |
@aimane-chnaif Thank you for pointing that out. |
🚀 Deployed to staging by https://github.com/francoisl in version: 1.2.78-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.2.78-0 🚀
|
Details
Combined with #14752, this PR completes the synchronisation of comment length counting with the backend.
An overview of the changes are:
getCommentLength
function.When comment length is exceeded, the
ExceededCommentLength
component callssetExceededCommentLength
on the parent composer components. This applies the statehasExceededMaxCommentLength
to the composer which in turn displays the red borders and prevents the message from being sent.Fixed Issues
$ #14268
PROPOSAL: #14268 (comment)
Tests
Offline tests
QA Steps
Use the following sequence of
*b*
characters for completing each test:Test 1 - Attempt sending MD that exceeds the limit
*b*
characters into the composer and remove any surrounding backticks and spaces.Test 2 - Attempt sending MD that does not exceed the limit
*b*
characters into the composer and remove any surrounding backticks and spaces.*b*
sequence from the comment. The comment should now be sendable.b
characters formatted in bold.Test 3 - Confirm composer counts the HTML length of the Markdown:
*b*
sequence above into the composer and remove any surrounding backticks and spaces.*b*
characters.*b*
sequence, the character counter increases by 18.Test 4 - Confirm comment is not counted until a break in typing (debounce functionality):
*b*
characters above into the composer and remove any surrounding backticks and spaces.Repeat the above tests for editing a comment. Confirm that comments exceeding the limit cannot be sent by pressing enter, or by clicking the send button.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
web.mp4
Mobile Web - Chrome
android_chrome.mp4
Mobile Web - Safari
ios_safari.mp4
Desktop
mac_native.mp4
iOS
ios_native.mp4
Android
android_native.mp4