-
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
[HOLD for payment 2024-10-25] [HOLD for payment 2024-10-24] [$250] Android - Room -On entering multiline text with header markdown, 2nd line is not visible #48281
Comments
Triggered auto assignment to @trjExpensify ( |
@trjExpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors |
Job added to Upwork: https://www.upwork.com/jobs/~0166150593d62a36c2 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @alitoshmatov ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.The room description on Android is cut off when typein multiple lines. What is the root cause of that problem?Line 1242 in 9e82ed8
function Line 1158 in 9e82ed8
What changes do you think we should make in order to solve the problem?function What alternative solutions did you explore? (Optional)NA |
Edited by proposal-police: This proposal was edited at 2023-10-01T00:00:00Z. ProposalPlease re-state the problem that we are trying to solve in this issue.Room -On entering multiline text with header markdown, 2nd line is not visible What is the root cause of that problem?To fix scroll bar flickering issue on line breaks we are the text input height here App/src/components/TextInput/BaseTextInput/index.tsx Lines 386 to 389 in e66b123
So the input has a fixed height so to allow autoGrow of height for multiline we are setting the height of the container to textInputHeight here
this textInputHeight is determined by the layout of an invisible Text here
But in our case the InputComponent is RNMarkdownTextInput which styles the text value based on the markdown and in this case when the user inputs header markdown it is bold and takes more space/width so jumps to the next line with fewer characters than where the normal invisible Text component discussed above which we use to calculate the textInputHeight .
What changes do you think we should make in order to solve the problem?The scroll-bar flickering problem is only for normal Lines 1229 to 1242 in e66b123
Lines 1158 to 1162 in e66b123
and apply same for native files too What alternative solutions did you explore? (Optional) |
ProposalPlease re-state the problem that we are trying to solve in this issue.The problem is that multi-line text is being cut off when you are using the heading ( What is the root cause of that problem?Now, there actually seems to be 2 different root causes for this issue.
rec1.mp4
rec2.mp4What changes do you think we should make in order to solve the problem?
The changes I made:
What alternative solutions did you explore? (Optional)An alternative solution that we can take in case we don't want to alter the Lines 1213 to 1222 in 76f16bc
(so, when it tries to 1.5x the lineHeight , it's not going to have an effect, but I still think my initial solution would be better)
|
ProposalPlease re-state the problem that we are trying to solve in this issue.Auto grow height of markdown text input doesn't grow properly, Like in room description text input. What is the root cause of that problem?The TextInput doens't have autoGrow functionality which make the baseTexinput autoGrowHeight of baseTextInput use hidden App/src/components/TextInput/BaseTextInput/index.tsx Lines 491 to 513 in fc849dd
in onLayout property the height and width of text input is adjusted. What changes do you think we should make in order to solve the problem?The react native live markdown has auto grow height functionality, we could use it and disable the usage of the hidden Text to detect the size of textInput widht/height. Some styling inside baseTextInput interfere with the auto grow of the markdown, so here is some modifications of the code: We add variable : const verticalPadding = useMemo (() => {
const flattenedInputStyle = StyleSheet.flatten(inputStyle);
const topBottomPadding = (flattenedInputStyle.paddingTop ?? 0) + (flattenedInputStyle.paddingBottom ?? 0);
if (topBottomPadding !== 0) {
return topBottomPadding;
}
return flattenedInputStyle.padding ?? 0;
}, [inputStyle]); This is needed, because the maxHeight style of the markdown doesn't include the padding, so we compute the vertical padding then subtract the maxAutoGrowHeight with the vertical padding.
We change it to: (autoGrowHeight && !isAutoGrowHeightMarkdown && styles.autoGrowHeightInputContainer(textInputHeight, variables.componentSizeLarge, typeof maxAutoGrowHeight === 'number' ? maxAutoGrowHeight : 0)) here:
change it to: <View style={[isAutoGrowHeightMarkdown ? undefined : styles.textInputAndIconContainer, isMultiline && hasLabel && styles.textInputMultilineContainer, styles.pointerEventsBoxNone]}> Then below this line:
we set the maxHeight of markdown: isAutoGrowHeightMarkdown? {maxHeight: maxAutoGrowHeight - verticalPadding} : undefined, Then change the conditional in:
to be Then we disable the hidden text input:
change it to : and in here:
change it to: Then do similarly in baseTextinput.native.tsx What alternative solutions did you explore? (Optional) |
@trjExpensify, @alitoshmatov Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
@alitoshmatov can you give these proposals a review, please? |
@jp928 Thank you for your proposal, not sure what you mean exactly but maxHeight is used to calculate input height based on text, padding and other factors. Setting it to |
@FitseTLT Thank you for your proposal, your RCA is correct. But your solution is not working on native app. Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-09-03.at.17.57.06.mp4 |
It's because Same change should be applied on .native files too that's why it is not working for native I have added comment on that @alitoshmatov |
@FitseTLT I think I added all needed changes I even checkout out to your provided branch before applying changes. Maybe I missed something, can you make sure changes are working in you ios native app and if yes provide me with detailed change for native too or you can update your branch |
@excile1 Thank you for your proposal, your RCA is correct. Your solution looks to be working fine, the 2nd bug you specified does not seem to be present in ios app. I just applied your first solution and ios app is working fine. Let me do some research for on the second bug and other platforms |
@alitoshmatov I didn't add the changes in native files in my branch, sorry that's why I included a note on my proposal to add it but now I have updated my test branch by include the same changes for native platforms 👍 U can check. |
Yep. It looks like the second bug is specific to android, it doesn't seem to be consistent with other platforms |
@FitseTLT Still not working, can you share a screen recording of it working on your ios app edit: Just checked, it is not working in android either. |
@tsa321 Thank you for your proposal, Your solution is working on ios fine, but have some issues on android Screen.Recording.2024-09-03.at.9.04.41.PM.mov |
@alitoshmatov might be a styling issue related to flex. I will try to revise my solution and inform you again. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.49-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2024-10-24. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.50-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-10-25. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
Payments are due tomorrow. Checklist time, please. Thanks! |
Bump on the checklist! |
Regression Test Proposal
|
Thanks! Confirming payments made as follows, accounting for a deduction for 1 regression:
This was caught with an existing regression test linked in the OP, so I'm not adding a new one. Paid, and closing! |
@justinpersaud @trjExpensify Be sure to fill out the Contact List! |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 9.0.26
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4904629
Issue reported by: Applause Internal Team
Action Performed:
Expected Result:
On entering multiline text with header markdown, 2nd line must be visible.
Actual Result:
On entering multiline text with header markdown, 2nd line is not visible.
Workaround:
Unknown
Platforms:
Screenshots/Videos
Bug6586126_1724906639755.Screenrecorder-2024-08-29-10-08-19-950_compress_1.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @trjExpensifyThe text was updated successfully, but these errors were encountered: