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

fix inline code blocks #2527

Merged
merged 8 commits into from
Apr 30, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
7 changes: 4 additions & 3 deletions src/components/AnchorForCommentsOnly/index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React from 'react';
import {StyleSheet} from 'react-native';
import {StyleSheet, Text} from 'react-native';
import anchorForCommentsOnlyPropTypes from './anchorForCommentsOnlyPropTypes';

const defaultProps = {
Expand All @@ -18,16 +18,17 @@ const AnchorForCommentsOnly = ({
style,
...props
}) => (
<a
<Text
style={StyleSheet.flatten(style)}
accessibilityRole="link"
href={href}
rel={rel}
target={target}
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
>
{children}
</a>
</Text>
);

AnchorForCommentsOnly.propTypes = anchorForCommentsOnlyPropTypes;
Expand Down
87 changes: 87 additions & 0 deletions src/components/InlineCodeBlock/WrappedText.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import React, {Fragment} from 'react';
import {Text, View} from 'react-native';
import PropTypes from 'prop-types';
import styles from '../../styles/styles';

/**
* Breaks the text into matrix
* for eg: My Name is Rajat
* [
* [My,'',Name,'','',is,'',Rajat],
parasharrajat marked this conversation as resolved.
Show resolved Hide resolved
* ]
*
* @param {String} text
* @returns {Array<String[]>}
*/
function getTextMatrix(text) {
return text.split('\n').map(row => row.split(/(\s)/));
}

const propTypes = {
// Required text
children: PropTypes.string.isRequired,

// Style to be applied to Text
// eslint-disable-next-line react/forbid-prop-types
textStyle: PropTypes.object,
parasharrajat marked this conversation as resolved.
Show resolved Hide resolved

// Style for each word(Token) in the text,
// remember that token also includes the following spaces before next word break
// eslint-disable-next-line react/forbid-prop-types
wordStyle: PropTypes.object,

// Style for first word
// eslint-disable-next-line react/forbid-prop-types
firstWordStyle: PropTypes.object,
parasharrajat marked this conversation as resolved.
Show resolved Hide resolved

// Style for last word
// eslint-disable-next-line react/forbid-prop-types
lastWordStyle: PropTypes.object,
};

const defaultProps = {
textStyle: {},
wordStyle: {},
firstWordStyle: {},
lastWordStyle: {},
};

const WrappedText = (props) => {
const textMatrix = getTextMatrix(props.children);
return (
<>
{textMatrix.map((rowText, rowIndex) => (
<Fragment
// eslint-disable-next-line react/no-array-index-key
key={`${rowText}-${rowIndex}`}
>
{rowText.map((colText, colIndex) => (

// Outer View is important to vertically center the Text
<View
// eslint-disable-next-line react/no-array-index-key
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain why this suppression is necessary? I'm confused because you are providing a key?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh. Sorry. Just made few adjustments and forget to Change it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, Eslint complains about an array index being used as key I know that is not the case but there is an error. So I had to

Copy link
Contributor

Choose a reason for hiding this comment

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

@parasharrajat Can you clarify what you mean here? What is the error?

Copy link
Member Author

@parasharrajat parasharrajat Apr 26, 2021

Choose a reason for hiding this comment

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

The exact error was Array index can't be used as a key. so I have to disable it to use the array index in the key.

key={`${colText}-${colIndex}`}
style={styles.codeWordWrapper}
>
<View
style={[
props.wordStyle,
colIndex === 0 && props.firstWordStyle,
colIndex === rowText.length - 1 && props.lastWordStyle,
]}
>
<Text style={props.textStyle}>{colText}</Text>
</View>
</View>
))}
</Fragment>
))}
</>
);
};

WrappedText.propTypes = propTypes;
WrappedText.defaultProps = defaultProps;
WrappedText.displayName = 'WrappedText';

export default WrappedText;
19 changes: 0 additions & 19 deletions src/components/InlineCodeBlock/index.android.js

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,23 +1,26 @@
/* eslint-disable react/jsx-props-no-spreading */
import React from 'react';
import {View} from 'react-native';
import styles from '../../styles/styles';
import WrappedText from './WrappedText';
import inlineCodeBlockPropTypes from './inlineCodeBlockPropTypes';

const InlineCodeBlock = ({
TDefaultRenderer,
defaultRendererProps,
boxModelStyle,
textStyle,
}) => (
<View
style={{
<WrappedText
textStyle={textStyle}
wordStyle={{
...boxModelStyle,
...styles.mbn1,
...styles.codeWordStyle,
}}
firstWordStyle={styles.codeFirstWordStyle}
lastWordStyle={styles.codeLastWordStyle}
// eslint-disable-next-line react/jsx-props-no-spreading
parasharrajat marked this conversation as resolved.
Show resolved Hide resolved
{...defaultRendererProps}
>
<TDefaultRenderer style={textStyle} {...defaultRendererProps} />
</View>
{defaultRendererProps.tnode.data}
</WrappedText>
);

InlineCodeBlock.propTypes = inlineCodeBlockPropTypes;
Expand Down
14 changes: 14 additions & 0 deletions src/styles/codeStyles/index.android.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
const codeWordWrapper = {
height: 10,
};

const codeWordStyle = {
top: -1,
marginVertical: -2,
};

const codeTextStyle = {
lineHeight: 18,
};

export default {codeWordWrapper, codeWordStyle, codeTextStyle};
16 changes: 16 additions & 0 deletions src/styles/codeStyles/index.ios.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
const codeWordWrapper = {
height: 18,
justifyContent: 'center',
marginTop: 2,
};

const codeWordStyle = {
height: 16,
top: 4,
};

const codeTextStyle = {
lineHeight: 15,
};
Comment on lines +12 to +14
Copy link
Contributor

Choose a reason for hiding this comment

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

The line height here was not enough and caused a regression #19922 when the inline code contains emojis.


export default {codeWordWrapper, codeWordStyle, codeTextStyle};
5 changes: 5 additions & 0 deletions src/styles/codeStyles/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// We do not need these on Web/Desktop as their implementation defer from Native devices so just noop them
const codeWordWrapper = {};
const codeWordStyle = {};
const codeTextStyle = {};
export default {codeWordWrapper, codeWordStyle, codeTextStyle};
39 changes: 35 additions & 4 deletions src/styles/styles.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import wordBreak from './utilities/wordBreak';
import textInputAlignSelf from './utilities/textInputAlignSelf';
import CONST from '../CONST';
import positioning from './utilities/positioning';
import codeStyles from './codeStyles';

const styles = {
// Add all of our utility and helper styles
Expand Down Expand Up @@ -1312,6 +1313,37 @@ const styles = {
scrollbarWidth: 'none',
},

codeWordWrapper: {
...codeStyles.codeWordWrapper,
},

codeWordStyle: {
borderLeftWidth: 0,
borderRightWidth: 0,
borderTopLeftRadius: 0,
borderBottomLeftRadius: 0,
borderTopRightRadius: 0,
borderBottomRightRadius: 0,
paddingLeft: 0,
paddingRight: 0,
justifyContent: 'center',
...codeStyles.codeWordStyle,
},

codeFirstWordStyle: {
borderLeftWidth: 1,
borderTopLeftRadius: 4,
borderBottomLeftRadius: 4,
paddingLeft: 5,
},

codeLastWordStyle: {
borderRightWidth: 1,
borderTopRightRadius: 4,
borderBottomRightRadius: 4,
paddingRight: 5,
},

fullScreenLoading: {
backgroundColor: themeColors.componentBG,
opacity: 0.8,
Expand All @@ -1332,8 +1364,6 @@ const styles = {
const baseCodeTagStyles = {
borderWidth: 1,
borderRadius: 5,
marginTop: 4,
marginBottom: 4,
borderColor: themeColors.border,
backgroundColor: themeColors.textBackground,
};
Expand Down Expand Up @@ -1388,11 +1418,11 @@ const webViewStyles = {

code: {
...baseCodeTagStyles,
...codeStyles.codeTextStyle,
paddingLeft: 5,
paddingRight: 5,
paddingBottom: 2,
alignSelf: 'flex-start',
fontFamily: fontFamily.MONOSPACE,
fontSize: 13,
},

img: {
Expand All @@ -1405,6 +1435,7 @@ const webViewStyles = {
baseFontStyle: {
color: themeColors.text,
fontSize: variables.fontSizeNormal,
lineHeight: variables.fontSizeNormalHeight,
fontFamily: fontFamily.GTA,
},
};
Expand Down
1 change: 1 addition & 0 deletions src/styles/variables.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export default {
fontSizeLarge: 17,
fontSizeh1: 19,
iconSizeExtraSmall: 12,
fontSizeNormalHeight: 20,
iconSizeSmall: 16,
iconSizeNormal: 20,
iconSizeLarge: 24,
Expand Down