hoangnm / react-native-week-view

Week View Component for react-native
MIT License
322 stars 96 forks source link

BUG: React Native Web - onGridClick() not sending correct parameters #244

Open rdewolff opened 2 years ago

rdewolff commented 2 years ago

Describe the bug When using RNWV on react-native-web, when clicking on the grid, the parameters are not sent properly to the callback function.

Steps To Reproduce

  1. launch on the web
  2. register a onGridClick event
  3. param startHour is always set to 0 ...

Expected behavior

Environment:

pdpino commented 2 years ago

I haven't tried to reproduce this yet, will do soon.

@rdewolff have you tried with rn-week-view latest version? (v0.18.0)

rdewolff commented 2 years ago

Nah I have tested on 0.16.0 only yet.

rdewolff commented 2 years ago

Seems like 0.18 is not working at all on react-native-web.

Screenshot 2022-08-01 at 22 00 37

pdpino commented 2 years ago

I'm able to reproduce the error Cannot read properties of undefined (reading 'Tap') in web by using react-native-gesture-handler: "~1.10.2". This error disappears if you upgrade react-native-gesture-handler to v2 (e.g. 2.5.0) (v2 is required for week-view)

However, when upgrading RNGH to v2.5.0 I run into this other RNGH bug in web: https://github.com/software-mansion/react-native-gesture-handler/issues/2056 (completely unrelated to week-view), so for now it won't work either :(

pdpino commented 2 years ago

I also reproduced the original issue (i.e. startHour being set to 0) in rn-week-view 0.16.0, it is a simple platform compatibility issue:

pdpino commented 2 years ago

@rdewolff I think week-view still has some issues to be fully supported in web (other example: scrolling #198).

For me, the priority right now is releasing some features for mobile. But supporting rn-web also would be awesome.

For anyone else interested, you can keep listing other necessary stuff to support rn-web (like this issue), and we can try to pick it up in the near future

rdewolff commented 2 years ago

It seems like on the web it's possible to use the event.nativeEvent.y prop to calculate the hour correctly.

rdewolff commented 2 years ago

On version 0.16.0, here is the working patched version if that can help:

diff --git a/node_modules/react-native-week-view/src/Events/Events.js b/node_modules/react-native-week-view/src/Events/Events.js
index 4b1cb13..17ce5e2 100644
--- a/node_modules/react-native-week-view/src/Events/Events.js
+++ b/node_modules/react-native-week-view/src/Events/Events.js
@@ -1,6 +1,6 @@
 import React, { PureComponent } from 'react';
 import PropTypes from 'prop-types';
-import { View, TouchableWithoutFeedback } from 'react-native';
+import { View, TouchableWithoutFeedback, Platform } from 'react-native';
 import moment from 'moment';
 import memoizeOne from 'memoize-one';

@@ -197,8 +197,15 @@ class Events extends PureComponent {
     }
     const { initialDate, hoursInDisplay, beginAgendaAt } = this.props;

+    // support different `nativeEvent` for browser 
+    let yValue
+    if (Platform.OS === 'web') {
+      yValue = event.nativeEvent.offsetY
+    } else {
+      yValue = event.nativeEvent.locationY
+    }
     const seconds = yToSeconds(
-      event.nativeEvent.locationY - CONTENT_OFFSET,
+      yValue - CONTENT_OFFSET,
       hoursInDisplay,
       beginAgendaAt,
     );
pdpino commented 2 years ago

@rdewolff are you staying in 0.16.0 in web and mobile? only web?

I'm wondering if is worth to make reanimated and gesture-handler optional dependencies, though it might be too much trouble

EDIT with more info: rn-reanimated and rn-gesture-handler handle these kind of web/mobile differences internally, so >= v0.17.0 solves this specific problem