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

[docs] Add a live demo with material-ui-pickers #13659

Closed
2 tasks
hanhtv204 opened this issue Nov 21, 2018 · 18 comments
Closed
2 tasks

[docs] Add a live demo with material-ui-pickers #13659

hanhtv204 opened this issue Nov 21, 2018 · 18 comments
Labels
docs Improvements or additions to the documentation

Comments

@hanhtv204
Copy link

  • I have searched the issues of this repository and
    believe that this is not a duplicate.
  • I have checked both on the new version (3.4) and old version (on my system 1.2.3). They are same issue

Expected Behavior 🤔

Should be show correct date when scroll to selecting date

Current Behavior 😯

Show invalid date

Steps to Reproduce 🕹

  1. Focus input
  2. Select month have 30 days (example: 11/2018)
  3. Focus into date value and scroll up => it show 11/31/2018. That's incorrect.

screenshot from 2018-11-21 10-20-40

Thank you.

@KaRkY
Copy link
Contributor

KaRkY commented Nov 21, 2018

@htran78 I think this is browser issue and not material-ui issue. As far as I know material-ui does not handle dates in any way. Even in documentation it says that it's a native picker.

@oliviertassinari oliviertassinari changed the title Invalid date input [docs] Add a live demo with material-ui-pickers Nov 21, 2018
@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. labels Nov 21, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 21, 2018

I would recommend going with https://github.com/dmtrKovalenko/material-ui-pickers by @dmtrKovalenko.

@dmtrKovalenko
Copy link
Member

I am on it!

@dmtrKovalenko
Copy link
Member

dmtrKovalenko commented Nov 21, 2018

@oliviertassinari I have started on integrating material-ui-pickers example to the docs. And figured out the unexpected trouble :(
I am not sure do I need to open a new issue. But maybe you can advice me smth. I am getting the following error

These dependencies were not found:

* @material-ui/core/Button in ./node_modules/material-ui-pickers/dist/material-ui-pickers
.esm.js
* @material-ui/core/Dialog in ./node_modules/material-ui-pickers/dist/material-ui-pickers
.esm.js
* @material-ui/core/DialogActions in ./node_modules/material-ui-pickers/dist/material-ui-
pickers.esm.js
...

For some reason material-ui docs are not resolving @material-ui/core path imports, that are using by.the pickers underhood.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 22, 2018

@dmtrKovalenko I can see that we are a peer dependency, which is great. It's the first time we have this configuration. The different options I can think of:

  • Install @material-ui/core as a peer dependency. I would be very carefully with this option 🥚🐓. It might block us in the future.
  • Tweek Next.js to let babel parse the material-ui-pickers folder. Can be challenging to make work.
  • Use Lerna modules aliasing. Can work out of the box :).

@dmtrKovalenko
Copy link
Member

@oliviertassinari are you talking about lerna link?

@oliviertassinari
Copy link
Member

@dmtrKovalenko I was able to have something working:

capture d ecran 2018-11-24 a 00 28 53

The code needs to be cleaned, I have been exploring all sorts of possible paths: oliviertassinari@c4fa588

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 27, 2018

Install @material-ui/core as a peer dependency. I would be very carefully with this option 🥚🐓. It might block us in the future.

I gave this statement another thought, maybe it's the opposite, using the source code directly might be an issue, especially if we start introducing breaking changes, in this case, relying on the version published on npm would be preferable. @eps1lon What do you prefer?

@dmtrKovalenko
Copy link
Member

dmtrKovalenko commented Nov 27, 2018

@oliviertassinari its a good point. But then all peer dependent packages may need different version of material-ui installed. I suggest just ping out and publish @next version of material-ui-pickers if some breaking changes will be introduced.

@oliviertassinari
Copy link
Member

But then all peer dependent packages may need different version of material-ui installed.

It's true, we can work around this problem by using the most recent version of the dependent package that matches the installed version of Material-UI, moving everything as a single bloc.

@dmtrKovalenko
Copy link
Member

@oliviertassinari that can be a tricky problem. Some packages can be not up-to-date much longer time than other packages. I suggest not publish every peer dependent package on demo.

@dmtrKovalenko
Copy link
Member

dmtrKovalenko commented Nov 27, 2018

If there any breaking changes introduced in @next which can affect material-ui-pickers work. You can ping me in the material-ui-pickers repo or here and I will make @next version with support of breaking change.

That will also make update to the new material-ui version painless for pickers users, `cause there will be much less time needs for updating pickers to the new version.

@eps1lon
Copy link
Member

eps1lon commented Nov 27, 2018

Install @material-ui/core as a peer dependency. I would be very carefully with this option eggrooster. It might block us in the future.

I gave this statement another thought, maybe it's the opposite, using the source code directly might be an issue, especially if we start introducing breaking changes, in this case, relying on the version published on npm would be preferable. @eps1lon What do you prefer?

That's what semver is for. If we don't introduce breaking changes in minor version upgrades then using something like ^3.1.0 as a peer dependency version is safe (if we stick to semver). If other libraries use >= 3.1.0 then they should stop using that unless they think it's a good idea to support any conceivable breaking change in the feature (e.g.potential @material-ui/[email protected]). This is then however not our issue.

I don't understand what you mean by "using the source code directly". If they use alpha releases or builds from master then this is their responsibility (again that's what semver is for).

@dmtrKovalenko I can keep an eye on material-ui-pickers compat but I would also suggest you subscribe to #13663 and let us know if something will cause much churn for you. Hopefully we can then work on a better solution. You should be safe most of the time tohugh if you fixed all deprecation warnings.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 27, 2018

"using the source code directly"

@eps1lon The issue is with how we should handle @material-ui/core. Should material-ui-pickers use the version from npm or from /packages/material-ui/src?

@eps1lon
Copy link
Member

eps1lon commented Nov 27, 2018

The same version our docs is using. It kind of adds another regression test for us which is great. Since material-ui-pickers uses ^3.2.0 it shouldn't block us either.

The issue here is that the path mappings are not working. I don't understand how they are working anyway right now in the docs:

  • docs use @material-ui/core/styles but I don't know where this is mapped to @material-ui/styles/src
  • docs use @material-ui/core which is mapped in the docs to @material-ui/core/src but why is this not applied to requires inside node_modules?

I would much rather fix this by putting the resolve mechanism into a single place than adding add dependency to our build process that is in 0.0.x.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 27, 2018

  1. We use dependency injection with install() from @material-ui/style. It's also what will allow us to have an emotion and styled components backend without bloating bundles.
  2. It's expected, Babel doesn't parse the node modules for performance considerations

The same version our docs is using.

In this case, I think that we are good to move forward.

@eps1lon
Copy link
Member

eps1lon commented Nov 27, 2018

babel transpilation and (webpack) resolve alias are two different things though.

@oliviertassinari
Copy link
Member

We don't use webpack alias, we use Babel alias.

@oliviertassinari oliviertassinari removed the good first issue Great for first contributions. Enable to learn the contribution process. label Dec 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

No branches or pull requests

5 participants