Skip to content

DateTimePicker - support light-date package (can replace moment) #2544

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

Closed
wants to merge 3 commits into from
Closed

DateTimePicker - support light-date package (can replace moment) #2544

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
efe910b
DateTimePicker - support light-date package (can replace moment)
M-i-k-e-l Mar 28, 2023
877ae11
Merge branch 'master' into feat/date-time-picker-support-light-date-p…
M-i-k-e-l Apr 20, 2023
2a4ea7d
Merge branch 'master' into feat/date-time-picker-support-light-date-p…
M-i-k-e-l Jun 13, 2023
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
19 changes: 18 additions & 1 deletion demo/src/screens/componentScreens/DateTimePickerScreen.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ export default class DateTimePickerScreen extends Component {
value={new Date('2015-03-25T12:00:00-06:30')}
/>
<Text text60R marginT-40>
Custom Input
Custom Input (date)
</Text>
<DateTimePicker
migrateTextField
Expand All @@ -71,6 +71,23 @@ export default class DateTimePickerScreen extends Component {
placeholder={'Select a date'}
renderInput={this.renderCustomInput}
dateFormat={'MMM D, YYYY'}
// useLightDate
// dateFormat={'{mm}-{dd}-{yyyy}'}
// value={new Date('2015-03-25T12:00:00-06:30')}
/>
<Text text60R marginT-40>
Custom Input (time)
</Text>
<DateTimePicker
migrateTextField
containerStyle={{marginVertical: 20}}
mode={'time'}
label={'Time'}
placeholder={'Select a date'}
renderInput={this.renderCustomInput}
timeFormat={'h:mm A'}
// useLightDate
// timeFormat={'{HH}:{mm}'}
// value={new Date('2015-03-25T12:00:00-06:30')}
/>
</View>
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@
"eslint-plugin-uilib": "file:./eslint-rules",
"gh-pages": "^1.1.0",
"jest": "^29.2.1",
"light-date": "^1.2.0",
"metro-react-native-babel-preset": "0.73.7",
"mocha": "^5.0.0",
"moment": "^2.24.0",
Expand Down
39 changes: 30 additions & 9 deletions src/components/dateTimePicker/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import React, {useCallback, useEffect, useMemo, useRef, useState} from 'react';
import {StyleProp, StyleSheet, ViewStyle} from 'react-native';
import {DateTimePickerPackage as RNDateTimePicker, MomentPackage as moment} from '../../optionalDependencies';
import {
DateTimePickerPackage as RNDateTimePicker,
MomentPackage as moment,
LightDatePackage as LightDate
} from '../../optionalDependencies';
import {useDidUpdate} from 'hooks';
import {Colors} from '../../style';
import Assets from '../../assets';
Expand Down Expand Up @@ -98,11 +102,15 @@ export type DateTimePickerProps = Omit<TextFieldProps, 'value' | 'onChange'> & {
* Should migrate to the new TextField implementation
*/
migrateTextField?: boolean;
}
/**
* Use the LightDate library instead of moment.
* Only send if you have both installed and you prefer using LightDate (moment is the default).
*/
useLightDate?: boolean;
Comment on lines +114 to +118
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that the advantage of using light-date is that we can install it as regular dependency (like lodash) and just use it.
If I remember correctly, users didn't want moment because it's a heavy dependency. We don't need to worry about it with light-date.

I would just swap the packages and use it directly instead of moment (and get rid of moment all together)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a problem because users (and us) will need to change the dateFormat`timeFormat` - you can see in the example screen it is very different; moreover it might give different results which will need UX approval.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm I see your point. In that case, should we consider having a different API for the formatter?
maybe some sort of a function prop that formats the value? this way each can do whatever and we can set the default behavior (using moment or whatever) in private uilib. WDYT?

Copy link
Collaborator Author

@M-i-k-e-l M-i-k-e-l Apr 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure if that's what you meant but we can remove (deprecate) all four formatting props (dateFormat, timeFormat, dateFormatter, timeFormatter) and introduce a single one formatter here:

const getStringValue = () => {
  if (value) {
    if (formatter) {
      return formatter(value);
    }

This will eliminate any need for dependencies.
Unless you know why we should keep the two formatter types separated (the user knows what mode they're in).

Copy link
Collaborator

Choose a reason for hiding this comment

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

But can you have a single formatter for all types of values? (date, time,...)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does DateTimePicker inherits all TextField props? You mean there's a name conflict?
Maybe call it dateTimeFormatter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it extends it and passes them all.
I thought about it, I don't like it as much and was hoping you'll say that probably no one is using formatter :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess no one does, but not sure I want to go into an adventure of name conflicts

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, do we want to add a TODO to change to formatter in the next major version or not?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think just leave it for now.. it's not crucial
Let's add the new prop and push this PR forward

};

type DateTimePickerPropsInternal = DateTimePickerProps & BaseComponentInjectedProps;


/*eslint-disable*/
/**
* @description: Date and Time Picker Component that wraps RNDateTimePicker for date and time modes.
Expand Down Expand Up @@ -137,6 +145,7 @@ function DateTimePicker(props: DateTimePickerPropsInternal) {
useCustomTheme,
testID,
migrateTextField = true,
useLightDate,
...others
} = props;

Expand All @@ -146,14 +155,26 @@ function DateTimePicker(props: DateTimePickerPropsInternal) {

useEffect(() => {
if (!RNDateTimePicker) {
// eslint-disable-next-line max-len
console.error(`RNUILib DateTimePicker component requires installing "@react-native-community/datetimepicker" dependency`);
}
}, []);

const format =
moment && (!LightDate || !useLightDate)
? // @ts-expect-error
(date: Date, format: string) => moment(date).format(format)
: LightDate
? // @ts-expect-error
(date: Date, format: string) => LightDate.format(date, format)
: undefined;

useEffect(() => {
if (!moment && (dateFormat || timeFormat)) {
console.error(`RNUILib DateTimePicker component with date/time format requires installing "moment" dependency`);
if (!format && (dateFormat || timeFormat)) {
// eslint-disable-next-line max-len
console.error(`RNUILib DateTimePicker component with date/time format requires installing "moment" or "light-date" dependency`);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [dateFormat, timeFormat]);

useDidUpdate(() => {
Expand Down Expand Up @@ -184,14 +205,14 @@ function DateTimePicker(props: DateTimePickerPropsInternal) {
case MODES.DATE:
return dateFormatter
? dateFormatter(value)
: dateFormat && moment
? moment(value).format(dateFormat)
: dateFormat && format
? format(value, dateFormat)
: value.toLocaleDateString();
case MODES.TIME:
return timeFormatter
? timeFormatter(value)
: timeFormat && moment
? moment(value).format(timeFormat)
: timeFormat && format
? format(value, timeFormat)
: value.toLocaleTimeString();
}
}
Expand Down
6 changes: 6 additions & 0 deletions src/optionalDependencies/LightDatePackage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
let LightDatePackage: typeof import('light-date') | undefined;
try {
LightDatePackage = require('light-date');
} catch (error) {}

export default LightDatePackage;
1 change: 1 addition & 0 deletions src/optionalDependencies/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ export {default as DateTimePickerPackage} from './DateTimePickerPackage';
export {default as FlashListPackage} from './FlashListPackage';
export {default as BlurViewPackage} from './BlurViewPackage';
export {default as MomentPackage} from './MomentPackage';
export {default as LightDatePackage} from './LightDatePackage';
export {default as NetInfoPackage} from './NetInfoPackage';
export {default as HapticFeedbackPackage} from './HapticFeedbackPackage';
export {default as SvgPackage} from './SvgPackage';
Expand Down