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.

Sign up for GitHub

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

Adding customCancelButton and customConfirmButton types #656

Merged
merged 3 commits into from
Sep 6, 2022
Merged

Adding customCancelButton and customConfirmButton types #656

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
18cf7be
Add customCancelButton & customConfirmButton types
luizaugustoventura Aug 28, 2022
91bc2d2
CancelButton & ConfirmButton as FunctionComponent
luizaugustoventura Aug 30, 2022
f089eab
isDarkModeEnabled optional for custom iOS buttons
luizaugustoventura Sep 2, 2022
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
97 changes: 84 additions & 13 deletions typings/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,80 @@ import {
AndroidNativeProps,
} from "@react-native-community/datetimepicker";

export type CancelButtonComponent = React.ComponentType<{
isDarkModeEnabled: boolean;
onPress(): void;
label: string;
}>;

export type ConfirmButtonComponent = React.ComponentType<{
isDisabled: boolean;
onPress(): void;
label: string;
}>;
export type CancelButtonStylePropTypes = {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure why we're exporting these styles. Froma a quick check it doesn't seem they will be used anyway, since style is not passed down to the custom component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, your point makes sense. But when I was doing these modifications, what I first did was importing the cancelButtonStyles and passing to a constant in my code; I modified this constant (just like I showed in the in the Test Plan) and then passed it to the style of the CancelButton on the customCancelButtonIOS property. I got this solution here.

All I wanted to do was to change the color of the CancelButton to red. So I did all of this without changing any line of code of your lib and everything worked just like the way I expected. But as I was on a TS project, the editor showed me some errors.

As I said, I just wanted to change the color of the CancelButton. But I decided to do these changes to extend the possibilities, just in case somebody wanted to change some other properties of the button, so I exported the format of the styles so that the code editor would show them how exactly the object looks like.

I did the modifications based on this issue. I did exactly what was done there, but got TypeScript errors. That's why I created this PR. Could you please take a look back there?

Copy link
Owner

Choose a reason for hiding this comment

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

Ah! Gotcha. Could you share the TypeScript issue you were having?
If the issue is that TypeScript complains for using properties not listed in ConfirmButtonStylePropTypes, I think the right solution would be to relax the ConfirmButtonStylePropTypes so that we allow any prop other than the one we're currently passing down to into it. Let me know if it makes sense!

Copy link
Contributor Author

@luizaugustoventura luizaugustoventura Aug 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Alright, there we go. I'm going to separate this into 2 approaches.

  1. Styling the CancelButton
import DateTimePicker, { CancelButton, cancelButtonStyles } from 'react-native-modal-datetime-picker';

const customCancelButtonStyles = {
  ...cancelButtonStyles,
  text: {
    ...cancelButtonStyles.text,
    color: 'red',
  }
};

return (
   <RNModalDateTimePicker
       // ...
       customCancelButtonIOS={(props) => <CancelButton {...props} style={customCancelButtonStyles} /> }
   />
);

I get what I want by doing this. But I see some problems:

  • The editor will not tell me anything about the style in CancelButton, so it will be taken as any. No TypeScript error here, but I think it's nice to have a type, so we know exactly what this component is taking in that prop;
  • The cancelButtonStyles is of type any. Again, this shows no error, but I think it would be very nice if we could see how exactly the original style of the component looks like. That is useful when we want to do slight changes to the component's styles, so we are going to use the same style properties, just like it's my case in the Test Plan;
  • And last and more importantly, on the imports I get two errors: 
    •  import CancelButton

 Module '"react-native-modal-datetime-picker"' has no exported member 'CancelButton'. Did you mean to use 
 'import CancelButton from "react-native-modal-datetime-picker"' instead?ts(2614)

    • import cancelButtonStyles

Module '"react-native-modal-datetime-picker"' has no exported member 'cancelButtonStyles'. Did you mean to 
use 'import cancelButtonStyles from "react-native-modal-datetime-picker"' instead?ts(2614)

Note that the style property does not light up an error only because the CancelButton is not being exported from the declaration file, so the editor assumes CancelButton as any. But if we export the CancelButton as CancelButtonComponent, the editor certainly will say that this component does not take the property style.

  1. Using the buttonTextColorIOS
import DateTimePicker, { CancelButton } from 'react-native-modal-datetime-picker';

return (
   <RNModalDateTimePicker
       // ...
       customCancelButtonIOS={(props) => <CancelButton {...props} buttonTextColorIOS="purple" /> }
   />
);

The editor will say the same thing about the CancelButton import. I know the second approach makes a lot more sense for my case, but someone (or even me) might change something else in another occasion.

In addition, the buttonTextColorIOS does not light up a red in my editor only because the CancelButton is assumed as any, as it is not being exported. But just like I said about the afore mentioned CancelButtonComponent, the editor will say CancelButton does not take the property buttonTextColorIOS.

Now going to your point, relaxing the ConfirmButtonStylePropTypes is complicated because if you look at the the way the CancelButton is being styled, you can see the styles object is being used in a very specific way as it is applied to both the Buton and its Text.

I know all of this looks bloat code maybe, but I just wanted to create more styling possibilities (something you could perfectly do in a JS project) for TS developers. That is my second ever contribution to an Open Source project, besides I created this one at the same day I created the first one.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for your detailed reply!
Before I start answering each bullet point separately, I wanted to make something clear.
The main discussion point here is that I think (and, again, I'm not sure about this, maybe I'm just missing something) we shouldn't mark the custom button styles as strict as we're doing in this PR.
With the current PR approach, we're basically saying that the custom button components styles can only be of the type ConfirmButtonStylePropTypes/CancelButtonStylePropTypes. Which is not true.

If you're using a custom button component, this library doesn't care about what style you pass to it.
Here's how it works for the confirm button, for example:

  1. If you provide a custom component, this library won't care about the internal confirm button
  2. It will just render the custom component as-it-is, without passing any style to it, just a few props.

Something like the following should be perfectly valid (just an example):

import DateTimePicker from 'react-native-modal-datetime-picker';
import { View, ViewStyle } from "react-native";

function MyCustomButton ({ style }: { style: ViewStyle }) {
  return <View style={style} />
}

// ... and then

return (
   <RNModalDateTimePicker
       customCancelButtonIOS={<MyCustomButton />}
   />
);

But with these PR changes it would break (I think so, right?) because we're saying that customCancelButtonIOS accepts only a component with a style prop that matches ConfirmButtonStylePropTypes... while in our example MyCustomButton has a style props that is just a ViewStyle.

Copy link
Owner

Choose a reason for hiding this comment

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

I know all of this looks bloat code maybe, but I just wanted to create more styling possibilities (something you could perfectly do in a JS project) for TS developers. That is my second ever contribution to an Open Source project, besides I created this one at the same day I created the first one.

No worries! I appreciate the contribution. Let's make sure the issue I'm mentioning is clear so that we can decide how to go forward.

P.S.: I'm also not a typing expert, and I didn't create the types of this component 😅
P.P.S.: I'm quite busy lately, so I might be slow to respond in the next days. Sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OMG! That's right!! 😅️😅️

I'm sorry I got carried away. I was only attempting to change the originally exported CancelButton and ConfirmButton but not passing my own component to customCancelButtonIOSor customConfirmButtonIOS.

Your point is absolutely clear for me now! I think I'd only set darkModeEnabled to optional to your CancelButtomComponent and ConfirmButtonComponent. So the type definition remained actually quite the same.
P.S.: I also changed the type's name, I don't know if that's a problem to you.

export type CustomCancelButtonPropTypes = {
  isDarkModeEnabled?: boolean,
  onPress: () => void,
  label: string,
};

export type CustomConfirmButtonPropTypes = {
  isDarkModeEnabled?: boolean,
  onPress: () => void,
  label: string,
};

Now, for the originally exported CancelButton and ConfirmButton I kept those properties (style included) because I think it would be handy, as these components aren't much flexible.

I'm still also exporting the CancelButtonStylePropTypes and ConfirmButtonStylePropTypes because I think they're still useful in case we want to do some slight changes to the original CancelButton and ConfirmButton components. Also, for both of these components I think it's interesting yet to set their style property strictly as CancelButtonStylePropTypes and ConfirmButtonStylePropTypes because both CancelButton and ConfirmButton's styles are being used in a very specific (take a look here and right here) way. Some keys of the style object are being used in a TouchableHighlight and others in a Text.

But with these PR changes it would break (I think so, right?) because we're saying that customCancelButtonIOS > accepts only a component with a style prop that matches ConfirmButtonStylePropTypes... while in our example > MyCustomButton has a style props that is just a ViewStyle.

Yes, it would. I tested them the way you explained. That's why I'm pushing a new commit to my PR with the changes I explained above is this comment. Please check it if makes sense to you now.

P.P.S.: I'm quite busy lately, so I might be slow to respond in the next days. Sorry.

Same situation here. Thank you so much for you time 😉️

button: {
borderRadius: number,
height: number | string,
marginBottom: number | string,
justifyContent: string,
},
buttonLight: {
backgroundColor: string,
},
buttonDark: {
backgroundColor: string,
},
text: {
padding: number | string,
textAlign: string,
color: string,
fontSize: number,
fontWeight: string,
backgroundColor: string,
},
};

export type ConfirmButtonStylePropTypes = {
button: {
borderTopWidth: number,
backgroundColor: string,
height: number | string,
justifyContent: string,
},
buttonLight: {
borderColor: string,
},
buttonDark: {
borderColor: string,
},
text: {
textAlign: string,
color: string,
fontSize: number,
fontWeight: string,
backgroundColor: string,
},
};

export type CancelButtonPropTypes = {
isDarkModeEnabled?: boolean,
cancelButtonTestID?: string,
onPress: () => void,
label: string,
buttonTextColorIOS?: string,
style?: CancelButtonStylePropTypes,
Copy link
Owner

Choose a reason for hiding this comment

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

Same as above, I don't think we're passing this down to the custom cancel/confirm components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I tried changing the buttonTextColorIOS and the style. Both worked like a charm.

As I said in the previous conversation, all I wanted was to change the button color. But these other properties might be useful for someone in the future.

For example: Maybe before cancel or confirm you want to prompt an Alert.

};

export type ConfirmButtonPropTypes = {
isDarkModeEnabled?: boolean,
confirmButtonTestID?: string,
onPress: () => void,
label: string,
buttonTextColorIOS?: string,
style?: ConfirmButtonStylePropTypes,
};

export type CustomCancelButtonPropTypes = {
isDarkModeEnabled?: boolean,
onPress: () => void,
label: string,
};

export type CustomConfirmButtonPropTypes = {
isDarkModeEnabled?: boolean,
onPress: () => void,
label: string,
};

export type HeaderComponent = React.ComponentType<{
label: string;
Expand Down Expand Up @@ -65,12 +128,12 @@ export interface DateTimePickerProps {
/**
* A custom component for the cancel button on iOS
*/
customCancelButtonIOS?: CancelButtonComponent;
customCancelButtonIOS?: React.FunctionComponent<CustomCancelButtonPropTypes>;

/**
* A custom component for the confirm button on iOS
*/
customConfirmButtonIOS?: ConfirmButtonComponent;
customConfirmButtonIOS?: React.FunctionComponent<CustomConfirmButtonPropTypes>;

/**
* A custom component for the title container on iOS
Expand Down Expand Up @@ -232,3 +295,11 @@ export default class DateTimePicker extends React.Component<
ReactNativeModalDateTimePickerProps,
any
> {}

export const cancelButtonStyles: CancelButtonStylePropTypes;

export const CancelButton: React.FunctionComponent<CancelButtonPropTypes>;

export const confirmButtonStyles: ConfirmButtonStylePropTypes;

export const ConfirmButton: React.FunctionComponent<ConfirmButtonPropTypes>;