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

Long lines in code blocks should not wrap issue (#7497) is fixed #8790

Merged
merged 21 commits into from
May 9, 2022
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import React, {forwardRef} from 'react';
import {ScrollView} from 'react-native-gesture-handler';
import {View} from 'react-native';
import _ from 'underscore';
import htmlRendererPropTypes from '../htmlRendererPropTypes';

class BasePreRenderer extends React.Component {
Copy link
Member

Choose a reason for hiding this comment

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

this is still class component. 😕

Copy link
Contributor Author

@orkunkarakus orkunkarakus May 3, 2022

Choose a reason for hiding this comment

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

Looks like a perfect candidate for the functional components. Please read the style guide and correct all style issues.

@parasharrajat I interpreted your sentence as this style is very good for functional components but not good for classes.
And it was functional before and we converted it to class, so I never looked in that direction and looked at the styling guide again and again to see where is a mistake 😃
i converted and committed Pr is ready.

render() {
const TDefaultRenderer = this.props.TDefaultRenderer;
const defaultRendererProps = _.omit(this.props, ['TDefaultRenderer']);
return (
<ScrollView ref={this.props.innerRef} horizontal>
<View onStartShouldSetResponder={() => true}>
<TDefaultRenderer
// eslint-disable-next-line react/jsx-props-no-spreading
{...defaultRendererProps}
/>
</View>
</ScrollView>
);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Looks like a perfect candidate for the functional components. Please read the style guide and correct all style issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like a perfect candidate for the functional components. Please read the style guide and correct all style issues.

@parasharrajat i refactored. Please re-check


BasePreRenderer.propTypes = htmlRendererPropTypes;
NikkiWines marked this conversation as resolved.
Show resolved Hide resolved

export default forwardRef((props, ref) => (
// eslint-disable-next-line react/jsx-props-no-spreading
<BasePreRenderer {...props} innerRef={ref} />
));
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import React from 'react';
import withLocalize from '../../../withLocalize';
import htmlRendererPropTypes from '../htmlRendererPropTypes';
import BasePreRenderer from './BasePreRenderer';

class PreRenderer extends React.Component {
constructor(props) {
super(props);

this.wheelEvent = this.wheelEvent.bind(this);
this.ref = React.createRef(null);
orkunkarakus marked this conversation as resolved.
Show resolved Hide resolved
}

componentDidMount() {
this.ref.current
.getScrollableNode()
Copy link
Contributor

Choose a reason for hiding this comment

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

this.ref.getScrollableNode() for stylistic consistency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored

.addEventListener('wheel', this.wheelEvent);
}

componentWillUnmount() {
this.ref.getScrollableNode()
.removeEventListener('wheel', this.wheelEvent);
}

wheelEvent(event) {
Copy link
Member

@parasharrajat parasharrajat May 3, 2022

Choose a reason for hiding this comment

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

Rename this function. The function name should define what it does instead of where it used.
scrollNode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@parasharrajat I changed the function name to scrollNode

const node = this.ref.current.getScrollableNode();
const checkOverflow = (node.scrollHeight / node.scrollWidth) !== (node.offsetHeight / node.offsetWidth);
Copy link
Member

Choose a reason for hiding this comment

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

Does not seem correct. scrollWidth is used for horizontal scrolling and scrollHeight for vertical scrolling. Shouldn't this be like
HorizontalOverflow = node.scrollWidth > node.offsetWidth

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@parasharrajat i changed it like this


if ((event.currentTarget === node) && checkOverflow) {
node.scrollLeft += event.deltaX;
event.preventDefault();
event.stopPropagation();
}
}

render() {
return (
<BasePreRenderer
// eslint-disable-next-line react/jsx-props-no-spreading
{...this.props}
ref={this.ref}
/>
);
}
}

PreRenderer.propTypes = htmlRendererPropTypes;
PreRenderer.displayName = 'PreRenderer';

export default withLocalize(PreRenderer);
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import React from 'react';
import withLocalize from '../../../withLocalize';
import htmlRendererPropTypes from '../htmlRendererPropTypes';
import BasePreRenderer from './BasePreRenderer';

const PreRenderer = props => (
<BasePreRenderer
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
Copy link
Contributor

Choose a reason for hiding this comment

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

Props can be inline:

    // eslint-disable-next-line react/jsx-props-no-spreading
    <BasePreRenderer {...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.

Refactored.

/>
);

PreRenderer.propTypes = htmlRendererPropTypes;
PreRenderer.displayName = 'PreRenderer';

export default withLocalize(PreRenderer);
2 changes: 2 additions & 0 deletions src/components/HTMLEngineProvider/HTMLRenderers/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import AnchorRenderer from './AnchorRenderer';
import CodeRenderer from './CodeRenderer';
import EditedRenderer from './EditedRenderer';
import ImageRenderer from './ImageRenderer';
import PreRenderer from './PreRenderer';

/**
* This collection defines our custom renderers. It is a mapping from HTML tag type to the corresponding component.
Expand All @@ -14,4 +15,5 @@ export default {

// Custom tag renderers
edited: EditedRenderer,
pre: PreRenderer,
};
5 changes: 3 additions & 2 deletions src/components/PressableWithSecondaryInteraction/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {LongPressGestureHandler, State} from 'react-native-gesture-handler';
import SelectionScraper from '../../libs/SelectionScraper';
import * as pressableWithSecondaryInteractionPropTypes from './pressableWithSecondaryInteractionPropTypes';
import styles from '../../styles/styles';
import hasHoverSupport from '../../libs/hasHoverSupport';

/**
* This is a special Pressable that calls onSecondaryInteraction when LongPressed, or right-clicked.
Expand All @@ -31,7 +32,7 @@ class PressableWithSecondaryInteraction extends Component {
* @param {Object} e
*/
callSecondaryInteractionWithMappedEvent(e) {
if (e.nativeEvent.state !== State.ACTIVE) {
if ((e.nativeEvent.state !== State.ACTIVE) || hasHoverSupport()) {
return;
}

Expand Down Expand Up @@ -73,7 +74,7 @@ class PressableWithSecondaryInteraction extends Component {
onPressOut={this.props.onPressOut}
onPress={this.props.onPress}
ref={el => this.pressableRef = el}
// eslint-disable-next-line react/jsx-props-no-spreading
// eslint-disable-next-line react/jsx-props-no-spreading
{...defaultPressableProps}
>
{this.props.children}
Expand Down
12 changes: 12 additions & 0 deletions src/libs/hasHoverSupport/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/**
* Allows us to identify whether the platform is hoverable.
*
* @returns {Boolean}
*/

import * as Browser from '../Browser';

const hasHoverSupport = () => !Browser.isMobile();

export default hasHoverSupport;

9 changes: 9 additions & 0 deletions src/libs/hasHoverSupport/index.native.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/**
* Allows us to identify whether the platform is hoverable.
*
* @returns {Boolean}
*/

const hasHoverSupport = () => false;

export default hasHoverSupport;
4 changes: 2 additions & 2 deletions src/styles/styles.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,8 @@ const webViewStyles = {

pre: {
...baseCodeTagStyles,
paddingTop: 4,
paddingBottom: 5,
paddingTop: 12,
paddingBottom: 12,
paddingRight: 8,
paddingLeft: 8,
fontFamily: fontFamily.MONOSPACE,
Expand Down