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

Replace moment specifier with format/parse functions #518

Merged
merged 10 commits into from Nov 25, 2017

Conversation

3 participants
@TrySound
Contributor

TrySound commented Oct 15, 2017

This PR will remove moment dependency in favour of parse and format helpers which can be handled with library chosen in a team.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Oct 15, 2017

Codecov Report

Merging #518 into master will increase coverage by 0.51%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #518      +/-   ##
========================================
+ Coverage   99.48%   100%   +0.51%     
========================================
  Files          15     15              
  Lines         581    608      +27     
  Branches      123    129       +6     
========================================
+ Hits          578    608      +30     
+ Misses          3      0       -3
Impacted Files Coverage Δ
src/addons/MomentLocaleUtils.js 100% <100%> (ø) ⬆️
src/DayPickerInput.js 100% <100%> (+2.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5954f41...02f6bc9. Read the comment docs.

codecov bot commented Oct 15, 2017

Codecov Report

Merging #518 into master will increase coverage by 0.51%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #518      +/-   ##
========================================
+ Coverage   99.48%   100%   +0.51%     
========================================
  Files          15     15              
  Lines         581    608      +27     
  Branches      123    129       +6     
========================================
+ Hits          578    608      +30     
+ Misses          3      0       -3
Impacted Files Coverage Δ
src/addons/MomentLocaleUtils.js 100% <100%> (ø) ⬆️
src/DayPickerInput.js 100% <100%> (+2.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5954f41...02f6bc9. Read the comment docs.

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Oct 15, 2017

Owner

Thanks for this PR!

I was thinking to provide a default format, e.g. format could return the day as YYYY-MM-DD, and parse would convert it to a Date object. String(new Date()) is a bit too generic as it returns also the time.

I was reasoning about writing a small utility that could recognize D, DD, M, MM, YY and YYYY as format strings, so that format would accept also strings like DD-MM-YY and parse would convert them accordingly. The idea is to cover the most common cases without writing too much code.

Owner

gpbl commented Oct 15, 2017

Thanks for this PR!

I was thinking to provide a default format, e.g. format could return the day as YYYY-MM-DD, and parse would convert it to a Date object. String(new Date()) is a bit too generic as it returns also the time.

I was reasoning about writing a small utility that could recognize D, DD, M, MM, YY and YYYY as format strings, so that format would accept also strings like DD-MM-YY and parse would convert them accordingly. The idea is to cover the most common cases without writing too much code.

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Oct 15, 2017

Owner

@TrySound also I believe parse and format should receive the locale as second argument, which may come from the dayPickerProps.

const parse = (string, locale) => { return 'parsedDate' };
const format = (date, locale) => { return 'string' };

<DayPickerInput 
  dayPickerProps={{ locale: 'ru' }} 
  format={format} 
  parse={parse}
/>

This would help developers to implement their own i18n.

Owner

gpbl commented Oct 15, 2017

@TrySound also I believe parse and format should receive the locale as second argument, which may come from the dayPickerProps.

const parse = (string, locale) => { return 'parsedDate' };
const format = (date, locale) => { return 'string' };

<DayPickerInput 
  dayPickerProps={{ locale: 'ru' }} 
  format={format} 
  parse={parse}
/>

This would help developers to implement their own i18n.

@gpbl gpbl self-requested a review Oct 15, 2017

@gpbl gpbl added this to the v7.0.0 milestone Oct 15, 2017

@TrySound

This comment has been minimized.

Show comment
Hide comment
@TrySound

TrySound Oct 15, 2017

Contributor

Maybe better to pass date objects directly and use internal format and parser for simple specifer as defaults. So you don't need to expose these defaults to let users understand date.

Contributor

TrySound commented Oct 15, 2017

Maybe better to pass date objects directly and use internal format and parser for simple specifer as defaults. So you don't need to expose these defaults to let users understand date.

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Oct 15, 2017

Owner

Maybe better to pass date objects directly and use internal format and parser for simple specifer as defaults. So you don't need to expose these defaults to let users understand date.

Not sure I follow – what do you mean?

Owner

gpbl commented Oct 15, 2017

Maybe better to pass date objects directly and use internal format and parser for simple specifer as defaults. So you don't need to expose these defaults to let users understand date.

Not sure I follow – what do you mean?

@TrySound

This comment has been minimized.

Show comment
Hide comment
@TrySound

TrySound Oct 15, 2017

Contributor

<DateInput value={new Date(1992, 12, 20)} onChange={d => console.log(d.toISOString())} />
This will allow to work with date directly without parsing received one.

Contributor

TrySound commented Oct 15, 2017

<DateInput value={new Date(1992, 12, 20)} onChange={d => console.log(d.toISOString())} />
This will allow to work with date directly without parsing received one.

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Oct 15, 2017

Owner

OK, this is another change we can consider, and it makes sense – as it is somewhat related to #508 ("Move input related props to inputProps").

Until now, the value prop means "value" as in input fields, i.e. is expected to be a string. Once we close #508, we can do this. However, I'd open another PR for this change – on the master branch where moment is still a required dependency – to be sure we update the tests just for that change.

Owner

gpbl commented Oct 15, 2017

OK, this is another change we can consider, and it makes sense – as it is somewhat related to #508 ("Move input related props to inputProps").

Until now, the value prop means "value" as in input fields, i.e. is expected to be a string. Once we close #508, we can do this. However, I'd open another PR for this change – on the master branch where moment is still a required dependency – to be sure we update the tests just for that change.

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Oct 15, 2017

Owner

I also think we should open a v7 branch and merge these PR there.

There are still some fixes to do on v6 that I would like to publish before other breaking changes.

Owner

gpbl commented Oct 15, 2017

I also think we should open a v7 branch and merge these PR there.

There are still some fixes to do on v6 that I would like to publish before other breaking changes.

@PeterKottas

This comment has been minimized.

Show comment
Hide comment
@PeterKottas

PeterKottas Nov 5, 2017

Hi guys, where do we stand with this? Do we have a date in mind for v7? I rly think getting rid of this moment dep is high priority

PeterKottas commented Nov 5, 2017

Hi guys, where do we stand with this? Do we have a date in mind for v7? I rly think getting rid of this moment dep is high priority

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Nov 5, 2017

Owner

@PeterKottas me too :) The problem is that there's a pending minor release, I'm working on it right now.

I'd like to release this minor version where you can optionally opt out from moment. I.e., it defaults to moment, but if you specify format and parse attribute, it will not. I'm not sure if it is possible, however.

Owner

gpbl commented Nov 5, 2017

@PeterKottas me too :) The problem is that there's a pending minor release, I'm working on it right now.

I'd like to release this minor version where you can optionally opt out from moment. I.e., it defaults to moment, but if you specify format and parse attribute, it will not. I'm not sure if it is possible, however.

@PeterKottas

This comment has been minimized.

Show comment
Hide comment
@PeterKottas

PeterKottas Nov 5, 2017

Oh that sounds cool. You should definitely be able to do that. Just require the module, then check if it's define before you use it and fallback to some detetime format. I think you already have logic for that there. It would however need to remain a peer dependency which scares some people away. But it it's only for now, I'd go for it.

PeterKottas commented Nov 5, 2017

Oh that sounds cool. You should definitely be able to do that. Just require the module, then check if it's define before you use it and fallback to some detetime format. I think you already have logic for that there. It would however need to remain a peer dependency which scares some people away. But it it's only for now, I'd go for it.

@gpbl gpbl modified the milestones: v8.0.0, v7.0.0 Nov 25, 2017

@gpbl gpbl self-assigned this Nov 25, 2017

@gpbl gpbl merged commit b385ec9 into master Nov 25, 2017

3 checks passed

ci/circleci: checkout-and-test Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 99.48%)
Details
codecov/project 100% (+0.51%) compared to 5954f41
Details

@gpbl gpbl deleted the momentless branch Nov 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment