Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
92d3147
add handling for wrapped code text for native devices
cdOut Jul 8, 2024
ebeed08
fix lint issues
cdOut Jul 8, 2024
e8441ef
correct imports and prettier
cdOut Jul 8, 2024
6f2325b
Merge branch 'main' into @cdOut/code-report-fixed
cdOut Sep 9, 2024
5764928
split long singular words into mulltiple strings
cdOut Sep 10, 2024
e74c97e
fix prettier and lint errors
cdOut Sep 10, 2024
39a022f
Merge branch 'main' into @cdOut/code-report-fixed
cdOut Sep 10, 2024
44d0d7d
memoize fontSize and infer it from textStyles
cdOut Sep 11, 2024
f0aa075
fix lint errors and refactor code
cdOut Sep 11, 2024
dae171d
Merge branch 'main' into @cdOut/code-report-fixed
cdOut Sep 13, 2024
1a0e31b
Merge branch 'main' into @cdOut/code-report-fixed
cdOut Sep 20, 2024
00740c4
remove manual memoization in favor of react-compiler
cdOut Sep 26, 2024
d4e9810
Merge branch 'main' into @cdOut/code-report-fixed
cdOut Sep 26, 2024
513be86
Merge branch 'main' into @cdOut/code-report-fixed
cdOut Sep 30, 2024
5db19ff
Merge branch 'main' into @cdOut/code-report-fixed
cdOut Oct 1, 2024
c6756b3
Merge branch 'main' into @cdOut/code-report-fixed
cdOut Oct 2, 2024
a7b0e02
Merge branch 'main' into @cdOut/code-report-fixed
cdOut Oct 15, 2024
30a4176
Merge branch 'main' into @cdOut/code-report-fixed
cdOut Oct 28, 2024
89dc570
Merge branch 'main' into @cdOut/code-report-fixed
cdOut Oct 30, 2024
23e436b
add longer explaination comment
cdOut Oct 30, 2024
66d9dc5
add unit test for splitLongWord
cdOut Oct 30, 2024
e9c459b
fix prettier
cdOut Oct 30, 2024
5c66624
fix jsdoc lint errors
cdOut Oct 30, 2024
32ea3fc
fix unit test expected results
cdOut Oct 30, 2024
8b76cc9
remove unexpected output substring from test
cdOut Oct 30, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion src/components/InlineCodeBlock/WrappedText.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -40,14 +42,29 @@ function containsEmoji(text: string): boolean {
return CONST.REGEX.EMOJIS.test(text);
}

/**
* Splits long words into multiple strings
Copy link
Contributor

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.

Copy link
Contributor Author

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.

*/
function splitLongWord(word: string, maxLength: number): string[] {
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I'll switch to using those to determine the charWidth

const charsPerLine = Math.floor(windowWidth / charWidth);

const textMatrix = getTextMatrix(children).map((row) => row.flatMap((word) => splitLongWord(word, charsPerLine)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let use memo here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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
Expand Down
1 change: 1 addition & 0 deletions src/styles/variables.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ export default {
onboardingModalWidth: 500,
welcomeVideoDelay: 1000,
explanationModalDelay: 2000,
fontSizeToWidthRatio: getValueUsingPixelRatio(0.8, 1),

// The height of the empty list is 14px (2px for borders and 12px for vertical padding)
// This is calculated based on the values specified in the 'getGoogleListViewStyle' function of the 'StyleUtils' utility
Expand Down
Loading