Long lines in code blocks should not wrap issue (#7497) is fixed #8790
Conversation
melvin-bot
bot
requested review from
NikkiWines and
parasharrajat
and removed request for
a team
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
Although, the checkboxes are checked on the Contributor (PR Author) Checklist code is not following that. This checklist must be followed. Please update the PR and let me know when ready. |
@parasharrajat thanks for response. I refactored the pr but I read all of them one by one and I couldn't find the item that I didn't fit. Can you tell me which items I do not comply with? Because items often contain probability. For example "If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like |
| const isHoverableDevice = ((Platform.OS === 'web') || (Platform.OS === 'windows') || (Platform.OS === 'macos')); | ||
| if ((e.nativeEvent.state !== State.ACTIVE) || isHoverableDevice) { |
There was a problem hiding this comment.
Doing platform-specific code this way is not allowed. If you want to run some code on a platform use files like index.web.js or index.android.js or index.native.js etc. there are a few libs in src/libs folder which you can take as an example.
There was a problem hiding this comment.
Also, Could you please explain to me what are you trying to do here? I can offer alternatives.
There was a problem hiding this comment.
Thanks for the detailed response. My purpose here is not to create platform-based code, but to check whether the device is a hoverable device or not. On Hoverable device the Long press handler perceives it as holding it while scrolling and puts us in the action function. I disable the long press handler on hoverable devices with this control.
There was a problem hiding this comment.
At the same time, if I write this file separately for each platform like index.web.js or index.android.js or index.native.js the same file will be repeated for some platforms. Therefore there will be more than one of the same file.
There was a problem hiding this comment.
Ok, so you mean that you want to do something for Web and Desktop. So create like this
isHoverable
index.native.js returns false;
index.js returns true
There was a problem hiding this comment.
Yes it seems like a good idea, thanks. Then I convert it to like this and test it. After the tests, I will update the pr and let you know.
There was a problem hiding this comment.
A better name would be hasHoverSupport
There was a problem hiding this comment.
Changed the name and made 1 more commit 👍🏼
Retested after this update and found no issues.
@parasharrajat Thanks for the response. I fixed this issue. |
|
@parasharrajat I guess you didn't receive a notification because I tagged you later. Pr is ready for testing. |
|
Web (firefox) @shawnborton Does this look fine? When we hover over the scrollbar it covers the content. |
|
It looks like the scroll bar cuts off the content a little bit though. Is that expected? Perhaps that only happens on hover? |
|
Yeah, it only happens on hover. Not sure if this is expected. @orkunkarakus Do you have a solution for this? |
|
FWIW I think that's probably fine if it's only cutting off the text like that on hover. |
@parasharrajat Increasing the padding will be enough. Sended new commit for this bug. cc: @shawnborton
|
|
My concern with increasing the padding is that this might look strange without the scrollbar now? Can you show me what it looks like without the scrollbar and with the various paddings so we can compare? Thanks! |
@shawnborton sure. With old padding Padding increased |
|
Can you show me those same screenshots but without content that needs to horizontally scroll? |
@shawnborton sure. I updated my previous post |
| class BasePreRenderer extends React.Component { | ||
| 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> | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Looks like a perfect candidate for the functional components. Please read the style guide and correct all style issues.
There was a problem hiding this comment.
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
| .removeEventListener('wheel', this.wheelEvent); | ||
| } | ||
|
|
||
| wheelEvent(event) { |
There was a problem hiding this comment.
Rename this function. The function name should define what it does instead of where it used.
scrollNode.
There was a problem hiding this comment.
@parasharrajat I changed the function name to scrollNode
|
|
||
| wheelEvent(event) { | ||
| const node = this.ref.current.getScrollableNode(); | ||
| const checkOverflow = (node.scrollHeight / node.scrollWidth) !== (node.offsetHeight / node.offsetWidth); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@parasharrajat i changed it like this
| import _ from 'underscore'; | ||
| import htmlRendererPropTypes from '../htmlRendererPropTypes'; | ||
|
|
||
| class BasePreRenderer extends React.Component { |
There was a problem hiding this comment.
this is still class component. 😕
There was a problem hiding this comment.
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.
src/components/HTMLEngineProvider/HTMLRenderers/PreRenderer/BasePreRenderer.js
Show resolved
Hide resolved
| return; | ||
| } | ||
| this.ref | ||
| .getScrollableNode() |
There was a problem hiding this comment.
this.ref.getScrollableNode() for stylistic consistency
| const PreRenderer = props => ( | ||
| <BasePreRenderer | ||
| // eslint-disable-next-line react/jsx-props-no-spreading | ||
| {...props} |
There was a problem hiding this comment.
Props can be inline:
// eslint-disable-next-line react/jsx-props-no-spreading
<BasePreRenderer {...props} />| <View | ||
| onStartShouldSetResponder={() => true} | ||
| > | ||
| <TDefaultRenderer |
There was a problem hiding this comment.
Same as below, props can be inline
There was a problem hiding this comment.
Ah, @orkunkarakus sorry if I was unclear. I meant that you could do the following:
<View onStartShouldSetResponder={() => true}>
- <TDefaultRenderer
- // eslint-disable-next-line react/jsx-props-no-spreading
- {...defaultRendererProps}
- />
+ {/* eslint-disable-next-line react/jsx-props-no-spreading */}
+ <TDefaultRenderer {...defaultRendererProps} />
</View>There was a problem hiding this comment.
But moving <View onStartShouldSetResponder={() => true}> to a single line is also a good change 👍
| <View | ||
| onStartShouldSetResponder={() => true} | ||
| > | ||
| <TDefaultRenderer |
There was a problem hiding this comment.
Ah, @orkunkarakus sorry if I was unclear. I meant that you could do the following:
<View onStartShouldSetResponder={() => true}>
- <TDefaultRenderer
- // eslint-disable-next-line react/jsx-props-no-spreading
- {...defaultRendererProps}
- />
+ {/* eslint-disable-next-line react/jsx-props-no-spreading */}
+ <TDefaultRenderer {...defaultRendererProps} />
</View>| <View | ||
| onStartShouldSetResponder={() => true} | ||
| > | ||
| <TDefaultRenderer |
There was a problem hiding this comment.
But moving <View onStartShouldSetResponder={() => true}> to a single line is also a good change 👍
@NikkiWines Sorry, I thought it was View line. |
|
Going to merge (once tests pass) as @parasharrajat had previously approved and my suggestions were all stylistic changes. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by @NikkiWines in version: 1.1.58-0 🚀
|
|
🚀 Deployed to production by @AndrewGable in version: 1.1.60-3 🚀
|
Details
Issue is code blocks no scrolling in touchable platforms and long lines in code blocks wraping.
The problem in the mentioned issue link has been resolved with this pr and it works smoothly on all platforms.
Fixed Issues
$ https://github.com/Expensify/App/issues/7497
Tests
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followed/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
Screenshots
Web
web-expensify.mov
Mobile Web
Desktop
desktop-expensify.mov
iOS
ios-expensify.mp4
Android
android-expensify.mov