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

Support date and datetime-local inputs #480

Closed
wants to merge 1 commit into from

Conversation

Floriferous
Copy link
Contributor

@Floriferous Floriferous commented Oct 31, 2018

It'd be helpful for DateField to also support type: 'date' inputs, instead of only datefield-local. Currently, you can change the type, but the data does not appear, as it has to be formatted properly: YYYY-MM-DD.

I propose a working, though not very elegant solution, that checks for inputProps being passed to the material-ui Input component. (source)

So to use this you have to do (with simple schema):

new SimpleSchema({
  dateField: { type: Date, uniforms: { inputProps: { type: 'date' } } },
  dateTimeField: { type: Date },
})

It works in chrome, haven't tested it in other browsers yet.

@Floriferous Floriferous requested a review from radekmie as a code owner October 31, 2018 13:34
@codecov
Copy link

codecov bot commented Oct 31, 2018

Codecov Report

Merging #480 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #480   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         157    157           
  Lines        1401   1402    +1     
=====================================
+ Hits         1401   1402    +1
Impacted Files Coverage Δ
packages/uniforms-material/src/DateField.js 100% <100%> (ø) ⬆️

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 251099b...2e2d4f8. Read the comment docs.

@radekmie radekmie self-assigned this Oct 31, 2018
@radekmie radekmie added the Type: Feature New features and feature requests label Oct 31, 2018
@radekmie
Copy link
Contributor

Hi @Floriferous. The code itself is good. I'm just not sure if this is the right way. Right now, each theme consequently implements some date picker - such change tends to implement more and more full-blown ones for each theme. It's more of an opinion thing, just let me think about it.

Also, what about time? It's also easy achievable, in the same way. On the other way, it's easy to do so with modelTransform - have you thought about it in your case?

@Floriferous
Copy link
Contributor Author

You're right, I agree it doesn't feel like the right way to do this.

Ideally, I should be able to simply specify what html type this is, and then it should work. But with the required formatDate transform you apply on value it gets a bit more complex : /

@radekmie
Copy link
Contributor

radekmie commented Nov 2, 2018

OK, so let's implement this in each package (not in AntD as there's a custom picker).

@radekmie
Copy link
Contributor

Ping @Floriferous.

@Floriferous
Copy link
Contributor Author

Floriferous commented Nov 13, 2018

How do you suggest we do this:

  • Add support for native type='date', type='datetime-local', and type='time', even though we're manipulating the value with custom functions
  • Custom type: time, date, date-time
  • Other idea?

The schemas will probably have to be defined as follows:

new SimpleSchema({
  dateField: { type: String, uniforms: { type: 'date' } },
  dateTimeField: { type: String, uniforms: { type: 'time' } },
});

Or should we store them as Date types, without date (00-00-00-hh:mm:00)/without time respectively (dd-mm-yyyy-00:00:00)?

@radekmie
Copy link
Contributor

As a rule, custom component is the best here - it can handle the in (model ~> DOM) and out (DOM ~> model) logic, parametrized with props (defined in the schema or directly in props). Now, we'd like to enhance the current flaky implementation a bit to support also date. It's not covered on purpose, because one, there's a ton of better date/date-time/time pickers, two, it's not really cross-browser.

We have to chose now, if we want to support more than one as well or not. IMO datetime-local (currently supported) and date (this PR) are enough. If so, we have to do it in every theme.

There's even a special paragraph on that, on Material-UI docs:

We are currently falling back to native input controls. If you are interested in implementing or have implemented a rich Material Design Picker with an awesome UX, please, let us know on mui/material-ui#4787 and mui/material-ui#4796! We could add a link to or a demo of your project in the documentation. Here are some components that are promising:

material-ui-pickers: date pickers and time pickers.
material-ui-time-picker: time pickers.
material-ui-next-pickers: date pickers and time pickers.

⚠️ Native input controls support by browsers isn't perfect.

All in all, I'd rather not do it and always suggest to use an existing picker or create a custom field for that.

@Floriferous
Copy link
Contributor Author

Oh, so should we just close this PR then?

@radekmie
Copy link
Contributor

I thought about it once again and yes, I'm closing.

@radekmie radekmie closed this Nov 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New features and feature requests
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants