-
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
[Android / iOS] Code blocks are overflowing the app border #44953
Changes from 7 commits
92d3147
ebeed08
e8441ef
6f2325b
5764928
e74c97e
39a022f
44d0d7d
f0aa075
dae171d
1a0e31b
00740c4
d4e9810
513be86
5db19ff
c6756b3
a7b0e02
30a4176
89dc570
23e436b
66d9dc5
e9c459b
5c66624
32ea3fc
8b76cc9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,9 @@ import type {StyleProp, TextStyle, ViewStyle} from 'react-native'; | |
import {View} from 'react-native'; | ||
import Text from '@components/Text'; | ||
import useThemeStyles from '@hooks/useThemeStyles'; | ||
import useWindowDimensions from '@hooks/useWindowDimensions'; | ||
import {containsOnlyEmojis} from '@libs/EmojiUtils'; | ||
import variables from '@styles/variables'; | ||
import CONST from '@src/CONST'; | ||
import type ChildrenProps from '@src/types/utils/ChildrenProps'; | ||
|
||
|
@@ -40,14 +42,29 @@ function containsEmoji(text: string): boolean { | |
return CONST.REGEX.EMOJIS.test(text); | ||
} | ||
|
||
/** | ||
* Splits long words into multiple strings | ||
*/ | ||
function splitLongWord(word: string, maxLength: number): string[] { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a unit test for this function? With a handful of different sized strings. To make sure it always returns the array we expect. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added unit test for this function 👍 |
||
if (word.length <= maxLength) { | ||
return [word]; | ||
} | ||
|
||
return word.match(new RegExp(`.{1,${maxLength}}`, 'g')) ?? []; | ||
} | ||
|
||
function WrappedText({children, wordStyles, textStyles}: WrappedTextProps) { | ||
const styles = useThemeStyles(); | ||
const {windowWidth} = useWindowDimensions(); | ||
|
||
if (typeof children !== 'string') { | ||
return null; | ||
} | ||
|
||
const textMatrix = getTextMatrix(children); | ||
const charWidth = variables.fontSizeLabel * variables.fontSizeToWidthRatio; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should'nt we depend on the styles that are being passed down from props? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, I'll switch to using those to determine the |
||
const charsPerLine = Math.floor(windowWidth / charWidth); | ||
|
||
const textMatrix = getTextMatrix(children).map((row) => row.flatMap((word) => splitLongWord(word, charsPerLine))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let use memo here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you suggest we memoize here, just the textMatrix or do you think the other values as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think all values as they are solely used for textmarix. |
||
|
||
return textMatrix.map((rowText, rowIndex) => ( | ||
<Fragment | ||
|
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.
Can we add more to the comment here, explaining why it is that we have to do 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.
Of course, I've added a more elaborate explanation for it.