-
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
Truncation fix for Lack of maximum height for the description field #38890
Truncation fix for Lack of maximum height for the description field #38890
Conversation
this is awaiting review over at |
@shubham1206agra Please 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] |
@brandonhenry Can you create a temporary function here to test this flow? |
@shubham1206agra example added where the fix is meant to be applied. Used 300 as the limit |
@shubham1206agra any updates on this one 😄 |
@brandonhenry Can you merge main here? |
@shubham1206agra just merged it in |
@shubham1206agra let me know if you find anything on this, hopefully we can get this merged before Tuesday 🤞🏿 |
Not really. This approach has some weird bugs. I am going through https://github.com/huang47/nodejs-html-truncate/blob/master/lib/truncate.js to check whether this approach is viable. Your current approach doesn't truncate correctly, and there is some inconsistency to actual output. |
@shubham1206agra alrighty, did you have any examples of input that you saw weird output? I tested thoroughly and wasn't able to get anything weird, so just curious |
@brandonhenry This is one specific example. The blockquote is changing the truncation of text. |
@shubham1206agra do you actually notice any markdown breaking though? I did some digging here and I noticed that i had implemented this not how we want this to be. I updated and things are working very consistent. The steps are:
check it out now and let me know thoughts I tested with these inputs:
|
Co-authored-by: Shubham Agrawal <[email protected]>
Co-authored-by: Shubham Agrawal <[email protected]>
Co-authored-by: Shubham Agrawal <[email protected]>
@shubham1206agra this is ready to be merged |
@puneetlath Ping for the merge |
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.
Two very small comments from me to improve the variable naming and comments.
Co-authored-by: Puneet Lath <[email protected]>
Co-authored-by: Puneet Lath <[email protected]>
@puneetlath yep! added those in, good catch |
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.
Typecheck is failing. I think because of the comment I left.
@puneetlath yee pulled it down and fixed the issues 👍🏿 |
✋ 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. |
🚀 Deployed to staging by https://github.com/puneetlath in version: 9.0.25-0 🚀
|
1 similar comment
🚀 Deployed to staging by https://github.com/puneetlath in version: 9.0.25-0 🚀
|
@@ -477,8 +484,12 @@ function MenuItem( | |||
titleToWrap = html; | |||
} | |||
|
|||
if (shouldTruncateTitle) { | |||
titleToWrap = Parser.truncateHTML(titleToWrap, characterLimit, {ellipsis: '...'}); |
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.
We forgot to wrap the content in comment
tag, which broke this truncation for plain text. The issue is here
Details
Fixed Issues
$ #37357
PROPOSAL: #37357 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop