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

feat: add react-native support #47

Closed
wants to merge 1 commit into from

Conversation

byteab
Copy link

@byteab byteab commented Dec 26, 2023

No description provided.

Copy link
Contributor

@Feshchenko Feshchenko left a comment

Choose a reason for hiding this comment

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

Hey @TheEhsanSarshar, thanks for the PR.
I can't see any test coverage of the onPress this is not acceptable for the lib. Same as any.
Additionally left comments in the code.

},
}
: {
onPress(evt: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

any is not acceptable, please specify event type.

@@ -2,6 +2,7 @@ import { MouseEvent } from 'react';

export interface DPPropsGetterConfig extends Record<string, unknown> {
onClick?(evt?: MouseEvent<HTMLElement>, date?: Date): void;
onPress?(evt?: any, date?: Date): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

any is not acceptable.
Why we need to add additional prop on top of onClick for ReactNative. Why can't we pass onClick: onPress?

Copy link
Author

Choose a reason for hiding this comment

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

just wanted to avoid dependency to react native.
or maybe we can copy GestureResponderEvent from react native ?

Copy link
Author

Choose a reason for hiding this comment

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

onClick is not working in react native so using onPress is common, although here onClick will also work. but to avoid confusion included onPress as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Better to add react-native to the pearDependencies and specify type.

@@ -12,4 +13,5 @@ export interface DPPropGetter extends Record<string, unknown> {
'aria-disabled'?: boolean;
'aria-selected'?: boolean;
onClick?(evt: MouseEvent<HTMLElement>): void;
onPress?(evt: any): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Comment on lines +1 to +2
export const isWeb = false;
export const isNative = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

How will it work, react automatically will pick platform.native.ts or platform.ts based on ReactNavite or not?
Since this is buildable package I can't see how it will work, (I'm not an expert in React Native).

Copy link
Author

Choose a reason for hiding this comment

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

yeah bundlers will handle that file. and builded artifact also include that.
so on react native it only resolves .native file

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we need to create a separate build for the React Native

@Feshchenko
Copy link
Contributor

Closing because it seems like this feature will not work without changes to the bundling system

@Feshchenko Feshchenko closed this May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants