[WIP] Replace moment #218

dmtrKovalenko · 2018-02-13T13:46:26Z

🎉
This PR will replace moment from peer dependency of the project #61
I have moved all moment specific usages to the utils, used for persian pickers.
But now persian pickers are broken, @alitaheri I need your input on that

Also we need to update typings and move utils to the separate npm package.
Feel free to discuss all things you have found here.

cherniavskii · 2018-02-14T11:09:18Z

Awesome! 👍
I'll take a look at it later :)

alitaheri · 2018-02-14T13:20:55Z

@dmtrKovalenko Please don't replace the formatting functions. Jalaali needs other formatters to work. I mean: for Gregorian its YYYY but for Jalali its jYYYY. please revert back to using explicit functions like getYearText

dmtrKovalenko · 2018-02-14T14:23:34Z

@alitaheri Maybe we can match format strings in persian utils from 'jYYYY' -> 'YYYY', because it would be helpful to have common formatting method for both date-fns and moment

…ontext

…ests

dmtrKovalenko · 2018-02-20T19:51:58Z

@cherniavskii @alitaheri
I have finished probably all logic for date-fns injection. I will return back not generic formatting (via functions) for persian pickers tommorrow, so can you please review this PR so we will release it this week.
Thank you 🎉

cherniavskii

Sorry for taking so long to review this PR.
This PR is really huge (good work by the way), so I'm submitting first batch of review comments, I'll try to review more later ;)

cherniavskii · 2018-02-21T11:59:30Z

lib/src/_shared/WithUtils.jsx

+import React from 'react';
+import PropTypes from 'prop-types';
+
+const withUtils = () => (Component) => {


Looks like first wrapper function is unused, so we can remove it (unless it is there for a purpose).

- const withUtils = () => (Component) => { + const withUtils = (Component) => {

Then usage of withUtils will look like:

- withUtils()(Component) + withUtils(Component)

I leave it for have an ability customize the decorator in the future.

cherniavskii · 2018-02-21T12:03:16Z

lib/src/index.js

@@ -3,3 +3,9 @@ export { default as DatePicker } from './DatePicker/DatePickerWrapper';
 export { default as TimePicker } from './TimePicker/TimePickerWrapper';

 export { default as DateTimePicker } from './DateTimePicker/DateTimePickerWrapper';
+
+export { default as MuiPickerUtilsProvider } from './utils/MuiPickersUtilsProvider';


typo in export name

- MuiPickerUtilsProvider + MuiPickersUtilsProvider

cherniavskii · 2018-02-21T12:05:06Z

lib/__tests__/TimePicker/TimePickerWrapper.usage.tsx

@@ -2,6 +2,8 @@ import * as React from 'react'
 import { Fragment, Component } from 'react';
 import TimePickerWrapper  from '../../src/TimePicker/TimePickerWrapper';
 import { Moment } from 'moment'
+import { utilsToUse } from '../test-utils';
+import MuiUtilsProvider from '../../src/utils/MuiPickersUtilsProvider'


Let's unify provider naming by using MuiPickersUtilsProvider name everywhere

cherniavskii · 2018-02-21T12:09:16Z

lib/__tests__/test-utils.js

+  ? momentUtils
+  : dateFnsUtils;
+
+const getComponentWithUtils = Component => React.cloneElement(Component, { utils: utilsToUse });


why not using withUtils here

Because then shallow rendering would render just a HOC, instead of testing component

You can use enzyme dive method or use createShallow({ dive: true }) from` MUI test utils

cherniavskii · 2018-02-21T12:14:36Z

lib/package.json

@@ -40,11 +40,15 @@
    "react-dom": "^16.2.0"
  },
  "dependencies": {
+    "date-fns": "^1.29.0",


What do you think about making material-ui-pickers to not rely on date libraries at all?
We can add moment-utils and datefns-utils as peerDependencies (each with moment or dateutils dependencies accordingly)

Of course I will remove date-fns dependency and will let user to choose which utils to use moment or date-fns. And there would not peerDependecies at all

cherniavskii · 2018-02-21T12:19:47Z

lib/src/utils/date-fns-utils.js

+import getYear from 'date-fns/get_year';
+import isEqual from 'date-fns/is_equal';
+
+export default class DateFnsUtils {


why using class here? Isn't good ol' object better for this purpose?

Have an ability to extend this class and override some props (usefull for persian picker)

Easy to determine which utils are using via typeof

That makes more sense, but both may be done with object too

alitaheri

Great job @dmtrKovalenko Thanks 👍 👍

alitaheri · 2018-02-21T20:18:20Z

lib/typings/utils.ts

+  startOfDay(value: MaterialUiPickersDate): MaterialUiPickersDate;
+  endOfDay(value: MaterialUiPickersDate): MaterialUiPickersDate;
+  format(value: MaterialUiPickersDate): string;
+  formatNumber(number: number): MaterialUiPickersDate


shouldn't this return string?

alitaheri · 2018-02-21T20:19:55Z

lib/typings/utils.ts

+  isAfterYear(value: MaterialUiPickersDate, comparing: MaterialUiPickersDate): boolean;
+  startOfDay(value: MaterialUiPickersDate): MaterialUiPickersDate;
+  endOfDay(value: MaterialUiPickersDate): MaterialUiPickersDate;
+  format(value: MaterialUiPickersDate): string;


can you document what strings this function gets for formats (e.g. 'YYYY', 'MMM D', ...). helps others implement their own utils.

@alitaheri I think we should leave pure functions for rendering dates. Like in persian-utils now. It would be more convenient to users, who would be override existsing utils :)

format. Takes a second argument. The format string. I'm only saying it would be helpful to document the expected input fot format strings. How can that make it impure?

Oh, I didnt understand you, sorry)

alitaheri · 2018-02-21T20:21:03Z

lib/typings/utils.ts

+
+export interface Utils {
+  date(value: any): MaterialUiPickersDate;
+  addDays(value: MaterialUiPickersDate, count: Number): MaterialUiPickersDate;


count: number

alitaheri · 2018-02-21T20:22:06Z

lib/typings/utils.ts

@@ -0,0 +1,34 @@
+import { MaterialUiPickersDate } from './date'
+import { isEqual, isAfter, endOfDay, setHours } from 'date-fns';


change the file name to .d.ts and remove this import. I think this is redundant.

@alitaheri I have removed date-fns imports, have no idea how they appeared there :)
this file just a common interface for moment-utils and date-fns-utils

alitaheri · 2018-02-21T20:26:19Z

lib/__tests__/wrappers/ModalWrapper.test.js

@@ -10,7 +10,7 @@ describe('ModalWrapper', () => {
  });

  it('Should renders', () => {
-    console.log(component.debug()); // TODO REMOVE ME
+    // console.log(component.debug());


why comment and not just remove the line? it's not pretty 😅

Oh, thats just really helpfull to just uncomment line and have an ability to review component)

alitaheri · 2018-02-21T20:28:20Z

docs/src/Demo/Demo.jsx

@@ -123,7 +123,7 @@ class Demo extends Component {
            sourceFile="PersianPickers.jsx"
          >
            <PersianPickers />
-          </SourcablePanel>
+          </SourcablePanel> */}


Good idea 👍 I'll update my utils library and fix these demos 👍 👍

New build system

…lsProvider name

dmtrKovalenko · 2018-02-26T12:38:40Z

@alitaheri I have returned back all text formatting methods. And as far I understand have fixed all issues and finish with the feature at all. If you have any concerns let me a shout :)
So what we will do with persian pickers next?
All you need - its just import moment utils, extend them and override all displayed methods.

cherniavskii · 2018-02-27T12:06:14Z

README.md

+
+```jsx
+import { MuiPickersUtilsProvider } from 'material-ui-pickers';
+import MomentUtils from 'material-ui-picker/utils/moment-utils';


typo, should be material-ui-pickers, not material-ui-picker

cherniavskii · 2018-02-27T12:08:53Z

README.md

@@ -97,7 +110,7 @@ openToYearSelection | boolean | false | Open datepicker from year selection
 minDate | date | '1900-01-01' | Minimum selectable date
 maxDate | date | '2100-01-01' | Maximum selectable date
 onChange | func | required | Callback firing when date accepted
-returnMoment | boolean | true | Will return moment object in onChange
+returnMoment | boolean | true | Will return moment object in onChange (if moment utils using)


let's remove this prop (as proposed in #172) in order to improve API consistence

@cherniavskii I dont think its a good proposal, because a lot of project is using moment + pickers now and that will cause a lot of breaking changes because it will require all users to process native date.
Or you sugges always return moment for moment pickers and date for datefns?

Or you sugges always return moment for moment pickers and date for datefns

Yes, in case of date-fns returnMoment prop doesn't make any sense.

because a lot of project is using moment + pickers now and that will cause a lot of breaking changes

Well, there will be breaking changes anyway

because it will require all users to process native date

Yes, but they can use utils to do it.

This way we'll have consistent and predictable API. If we'll add utils for one more date processing library in the future - API will not change.

You maybe not fully understand the purpose of utils architecture? Its a way to users that work with moment pass moment objects and get moment objects from the pickers, and also not reduce bundle size by another date library.
So it just the best of utils - return moment if moment utils and date if date utils

Does returnMoment still make sense?

I think I will remove it and force passing moment everywhere if moment-utils provided

cherniavskii · 2018-02-27T12:09:38Z

README.md

@@ -165,7 +178,7 @@ rightArrowIcon | react node | `<Icon>keyboard_arrow_right</Icon>`| Right arrow i
 dateRangeIcon | react node | `<Icon>date_range</Icon>`| Date tab icon 
 timeIcon | react node | `<Icon>access_time</Icon>`| Time tab icon
 ampm | boolean | true | 12h/24h view for hour selection clock
-shouldDisableDate | (date: Moment) => boolean | () => false | Allow to disable custom date in calendar
+shouldDisableDate | (date: Moment | Date) => boolean | () => false | Allow to disable custom date in calendar


can we avoid passing moment object to this callback?

No. We are poviding moment for api consistent. Moment utils is a way to use moment in this callback.

what if we pass utils as second argument? This will allow data processing

@cherniavskii read my answer to your previous coment :)

cherniavskii · 2018-02-27T12:10:13Z

README.md

@@ -175,15 +188,6 @@ mask | text mask (read more [here](https://github.com/text-mask/text-mask/blob/m
 clearable | boolean | false | If `true`, clear button will be displayed
 TextFieldComponent | func, string | undefined | Component that should replace the default Material-UI TextField

-### l10n
-For l10n texts we're currently relying on moment which is stateful. To change the locale you have to import your langauge specific files an change the locale manually via `moment.locale(language)`.


Would be great to provide at least links to moment/date-fns l10n docs

It will be a part of next release

No updates, support of persian pickers would be a part of next release.

dmtrKovalenko · 2018-02-28T12:11:13Z

@cherniavskii
I want to merge this PR, test a bit new functionality and create a new release. (rc-1)
We will add support of localization at all and integrate persian pickers in the next release, so do you have any additional questions?

cherniavskii

Great, go ahead ;)

Move all moment specific usages to utils

90cb877

dmtrKovalenko requested review from alitaheri and cherniavskii February 13, 2018 13:46

dmtrKovalenko changed the title ~~🎉 Replace moment~~ [WIP] Replace moment Feb 13, 2018

dmtrKovalenko self-assigned this Feb 13, 2018

remcohaszing mentioned this pull request Feb 13, 2018

Replace moment with date-fns #219

Closed

1 task

dmtrKovalenko closed this Feb 14, 2018

dmtrKovalenko reopened this Feb 14, 2018

dmtrKovalenko added 7 commits February 20, 2018 14:33

Move missing moment usages to utils

fc9402d

Add date-fns utils

9f2d8fb

Add date-fns utils, imlement hoc for setting and getting utils from c…

e20ec44

…ontext

Improve utils, fix crashings

4245319

Add typings for utils and new components

47850ef

Add test setup for moment and date-fns separately, fix failing unit-t…

bb3051c

…ests

Fix not imported full componenets in some tests

46a1938

Remove console.log

2e2db64

dmtrKovalenko changed the base branch from master to develop February 21, 2018 07:41

dmtrKovalenko added 2 commits February 21, 2018 12:11

Change build system to direct import from components name (without src)

97ee95e

Fix unit tests

fad69c5

cherniavskii reviewed Feb 21, 2018

View reviewed changes

dmtrKovalenko added 2 commits February 21, 2018 14:49

Change all moment types to Date or Moment in type defenitions

e8e5ec1

Make date type unknown

1b567fd

alitaheri previously requested changes Feb 21, 2018

View reviewed changes

dmtrKovalenko added 3 commits February 22, 2018 09:29

Merge pull request #224 from dmtrKovalenko/new-build-system

3ba037c

New build system

Merge conflicts

dc16540

Fix utils interface typings errors, fix typo in exported MuiPickerUti…

9d6887c

…lsProvider name

dmtrKovalenko added 3 commits February 26, 2018 14:21

Add displaying texts methods to utils

36fa3e8

Merge conflicts

9d286e1

Reimplement range countings for moment manually, remove moment-range dep

4b18c46

dmtrKovalenko added 3 commits February 26, 2018 14:40

Change any date type to MaterialUIPickersDate

d2dae09

Update Readme.md to match new Utils architecture

d986820

Change moment type defenitions in proptypes table

51d34ac

cherniavskii reviewed Feb 27, 2018

View reviewed changes

dmtrKovalenko added 2 commits February 28, 2018 14:00

Fix typo, comment persian pickers docs section

51be3bb

Remove returnMoment prop

0006018

cherniavskii approved these changes Feb 28, 2018

View reviewed changes

dmtrKovalenko merged commit 7457129 into develop Feb 28, 2018

cherniavskii deleted the feature/replace-moment branch March 1, 2018 17:57

cherniavskii mentioned this pull request Mar 5, 2018

Refactor and Fix Jalali issues #241

Merged

benmccann mentioned this pull request May 24, 2018

Do not require moment.js chartjs/Chart.js#4303

Closed

		@@ -0,0 +1,34 @@
		import { MaterialUiPickersDate } from './date'
		import { isEqual, isAfter, endOfDay, setHours } from 'date-fns';

[WIP] Replace moment #218

[WIP] Replace moment #218

Conversation

dmtrKovalenko commented Feb 13, 2018 • edited Loading

cherniavskii commented Feb 14, 2018

alitaheri commented Feb 14, 2018

dmtrKovalenko commented Feb 14, 2018

dmtrKovalenko commented Feb 20, 2018

cherniavskii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alitaheri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmtrKovalenko commented Feb 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmtrKovalenko Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmtrKovalenko Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmtrKovalenko Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmtrKovalenko commented Feb 28, 2018

cherniavskii left a comment

Choose a reason for hiding this comment

dmtrKovalenko commented Feb 13, 2018 •

edited

Loading

dmtrKovalenko Feb 27, 2018 •

edited

Loading

dmtrKovalenko Feb 27, 2018 •

edited

Loading

dmtrKovalenko Feb 27, 2018 •

edited

Loading