-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[$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. |
It’s fine, @excile1. If my proposal is accepted, you can fully work on this issue using my proposed code changes. |
@trjExpensify, @alitoshmatov Eep! 4 days overdue now. Issues have feelings too... |
@alitoshmatov let us know your thoughts on next steps. 👍 |
I agree, I was planning to open a separate issue in the |
@tomekzaw Looks like you have worked on TLDR: The problem is in android where you can see little jump when heading markdown is written in the input. Screen.Recording.2024-09-03.at.9.04.41.PM.movThe solution proposed is:
|
Hi, author of react-native-live-markdown here. We're aware of this issue and we're working on a fix. This problem appears when both In RNLiveMarkdown there's a At this point we're still not sure why
As for little jump, this is actually a problem with React Native's edit: see #48281 (comment) |
Hey! I was helping @tomekzaw investigate the issue and what he said in the above message turned out not to be entirely true. The conflict between commit hooks from As was already stated in multiple proposals above, the root of the issue here is Regarding the issues from the video, the little jump when creating a heading:
is expected - until a letter is written after The other jump Screen.Recording.2024-09-19.at.13.30.08.movas @tomekzaw explained above is coming from React Native itself. It's also happening on pure I'd argue that both of these are outside the scope of this issue. |
As for the other jump mentioned by @j-piasecki, here's a video recording that compares |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@tomekzaw @j-piasecki Thank you both for these helpful insights. |
So I think we can conclude that this issue is outside of our scope and we should go on with our existing solution to fix height calculation problem. @tsa321 Since we are only using your solution, will you be able to work on this yourself? |
@alitoshmatov yes, if my proposal gets accepted, I can work on this. |
Triggered auto assignment to @justinpersaud, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
Sounds good to me! |
📣 @alitoshmatov 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
📣 @tsa321 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
@justinpersaud @trjExpensify @alitoshmatov @tsa321 this issue is now 4 weeks old, please consider:
Thanks! |
@alitoshmatov PR is ready |
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: @tsa321The text was updated successfully, but these errors were encountered: