fire hover event on supported devices only #20432
Conversation
melvin-bot
bot
requested review from
puneetlath and
Santhosh-Sellavel
and removed request for
a team
|
@puneetlath @Santhosh-Sellavel One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@kushu7 changes do not look like the second option from the proposal |
@Santhosh-Sellavel , As component got updated during Proposal Review so i did some modification, but its inherit same properties of 2nd option. no unnecessary callbacks for non supported devices
this.hasHoverSupport = DeviceCapabilities.hasHoverSupport();
if (_.isFunction(child)) {
child = child(this.state.isHovered && this.hasHoverSupport);
}
return this.hasHoverSupport ? (<WithHover/>) :( <WithoutHover/>)When i provided proposal. Component was like this: During proposal |
src/components/Hoverable/index.js
Outdated
| @@ -74,48 +78,52 @@ class Hoverable extends Component { | |||
| } | |||
|
|
|||
| if (_.isFunction(child)) { | |||
| child = child(this.state.isHovered); | |||
| child = child(this.state.isHovered && this.hasHoverSupport); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
we can use early return instead
There was a problem hiding this comment.
@Santhosh-Sellavel Thats good suggestion,
You are suggesting this?
if (_.isFunction(child)) {
child = child(this.state.isHovered && this.hasHoverSupport);
}
if(!this.hasHoverSupport) {
return <View>{child}</View>
}There was a problem hiding this comment.
Look if there is any better ways refactor if not lets just proceed with above
There was a problem hiding this comment.
let child = this.props.children;
if (_.isArray(this.props.children) && this.props.children.length === 1) {
child = this.props.children[0];
}
// return child if device doesn't has hoverSupport
if (!this.hasHoverSupport) {
const childrenWithHoverState = _.isFunction(child) ? child(false) : child;
return <View >{childrenWithHoverState}</View>;
}
if (_.isFunction(child)) {
child = child(this.state.isHovered);
}This looks good to me. as we are not doing && and ternary operation.
Just pushing code.
|
@Santhosh-Sellavel Just pushed latest changes. please have a look at it. |
src/components/Hoverable/index.js
Outdated
| @@ -73,6 +77,12 @@ class Hoverable extends Component { | |||
| child = this.props.children[0]; | |||
| } | |||
|
|
|||
| // return child if device doesn't has hoverSupport | |||
src/components/Hoverable/index.js
Outdated
| @@ -15,6 +17,8 @@ class Hoverable extends Component { | |||
| }; | |||
|
|
|||
| this.wrapperView = null; | |||
| // Skip hover on not supported devices like mWeb | |||
There was a problem hiding this comment.
can remove this comment too
There was a problem hiding this comment.
i have removed comments
|
@Santhosh-Sellavel Bump for review. Changes are pushed. Please review it once. |
|
It's not fixed yet able to reproduce easily on mWeb Android Screen_Recording_20230610_015353_Chrome.mp4 |
|
Investigating more I was able to find that this issue was not able to reproduce on iOS mWeb before or after this PR. I've added a custom style for the new view to make sure the new code was executed, and yet the issue occurs we are missing something else here. Screen.Recording.2023-06-10.at.2.08.38.AM.movAnother weird on real styles don't even appear 😨. I have only a Samsung device on that device, 
cc: @puneetlath |
May i know where do you put that styles? I will retest again. Thanks |
|
Here it is if (!this.hasHoverSupport) {
const childrenWithHoverState = _.isFunction(child) ? child(false) : child;
return <View style={{
borderColor: "red",
borderWidth: 1,
}} >{childrenWithHoverState}</View>;
} |
|
I debugged this. Now So overall my RCA was correct. still we are relying on Here is a video: problem persists. bug.mp4 |
@Santhosh-Sellavel Please see issue video once. You have to be quickly longPress after opening search page. I'm still able to reproduce it. Here is video: reproducing issue and resolution. bug_and_changes.mp4cc @puneetlath |
|
@tylerkaraszewski Please check the above thread and let us know your thoughts thanks! |
|
@Santhosh-Sellavel I experience the same problem I reported here #20585. The root cause is exactly the same (because of the |
|
Here the focus is on hovered style alone for now. |
|
@Santhosh-Sellavel any updates? |
|
Yeah, I am able to reproduce. The other PR seems to be a confusing factor here. Both are interdependent on each other here. One can not be tested without the other fix. So both should be done in the same PR if possible or the Other PR has to be reverted to merge this one. @kushu7 Do you have any solution for the other issue? |
Coming from this #19612 (comment). I was checking Pressable of I think we should not depend on |
|
Just tested after merging of PR #20838 @Santhosh-Sellavel What should be need to do now i can see 4 components are still utilizing cc @puneetlath |
|
Bump for suggestion. |
|
Discussing internally! |
|
Closing as discussed here: #19766 (comment) |
Details
should fire hover event on supported devices only. for that we are using
DeviceCapabilities.hasHoverSupport()Fixed Issues
$ #19766
PROPOSAL: #19766 (comment)
Tests
Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly 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)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
web.mov
Mobile Web - Chrome
mweb-chrome.mp4
Mobile Web - Safari
mWeb-safari.mov
Desktop
desktop1k.mov
iOS
ios.mov
Android
android.mp4