mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
91.86k stars 31.57k forks source link

Material-UI not compatible with React 0.14 #1030

Closed tleunen closed 8 years ago

tleunen commented 8 years ago

Because of the change on the refs that React 0.14 has. I think Material-UI will have quite a lot of issues with almost every component of the library.

the this.refs.XXX will return the DOM node instead of the component, which means we won't be able to call any component functions on it.

jkruder commented 8 years ago

@tleunen You're on the ball! React 0.14 beta was released today.

@hai-cea I took a look at some of the issues (I found ~50 across 21 files). This could be a good opportunity for a milestone. I was looking at the dialog/dialog-window/overlay files and there's a lot of coupling via this.refs.xxx.yyy() so this may not be a simple task. If you want to make a React 0.14 Compatibility milestone let me know and I can assist in writing issues (I'm sure @tleunen would like to be involved -- I don't want to volunteer him).

hai-cea commented 8 years ago

Thanks guys @tleunen!

I agree, this will cause some problems. What's the alternative? Save the element into a variable inside render?

@jkruder Are you on gitter - https://gitter.im/callemall/material-ui ? Please send me an IM on there and we'll get the ball rolling on organizing issues/milestones.

tleunen commented 8 years ago

Most of the time, components should not have public functions. Everything has to be passed with props or when they are mount.

For example, the Dialog component doesn't need show and hide. If it's in the dom, it's displayed, otherwise it's not.

I'm just starting using Material-UI so I'm not much aware of issues with others components, but I think most of them could be rewritten to not have public functions (Anyway there is no other choices). The thing is, it will be a big breaking change from the current version.

oliviertassinari commented 8 years ago

@tleunen I can't agree more on the Dialog show and hide methods.

hai-cea commented 8 years ago

@tleunen @oliviertassinari Yeh, I agree with you both. This was a debate we had when designing that component. The issue that we had was the clickaway feature. If open/close was handled in state, then it could worry about closing itself on clickaway. If open/close were passed in as props, everyone using dialog would have to handle clickaway themselves.

Now a middle solution would be to add an onClickAway prop to dialog and let the user of the component open/close.

oliviertassinari commented 8 years ago

@hai-cea I would suggest to use the same approche as https://github.com/rackt/react-modal. This is basicaly your middle solution.

mull commented 8 years ago

What other thing does this library use this.refs for when interacting with a DOM component than saying getDOMNode()? This change in React only applies to components such as <div/> and <i/> doesn't it? Your custom components can still be accessed by this.refs.xxx as per usual. Correct me if I'm wrong I haven't tried React 0.14 yet, but this same thing came up on HackerNews.

tleunen commented 8 years ago

You mean that if React detect the ref is on a custom component, it will return the component instead of the DOM element inside the component?

mull commented 8 years ago

@tleunen that is my understanding. Needs verification :)

hai-cea commented 8 years ago

@mull That would be very nice if that's the case. :)

jkruder commented 8 years ago

@hai-cea @mull @tleunen Just ran a quick test and ref on a custom (something that extends React.Component) component (this.refs.customComponent) will return a reference to the React component NOT the underlying DOM node. If you have a ref to a DOM node (div/a/img/etc) then this.refs.domRef will return the node.

@hai-cea That being said, I still think it's a good idea to move away from calling methods on this.refs.XXX.

mull commented 8 years ago

@jkruder thanks, glad I wasn't talking out of my ... :)

hai-cea commented 8 years ago

ok Thanks for the doing the research @jkruder. I think we're save to close this. Although, I think we still need to do a 0.14 milestone?

Also, what do you guys think about #1033 ?

tleunen commented 8 years ago

I guess we can close it then. But it should be good to rewrite some of the components to remove the need of calling functions on them. It's not how components should work :/

jkruder commented 8 years ago

@hai-cea Agreed. I'm working on a draft of the proposed work for 0.14 milestone that I'll send to you for feedback.

In regards to #1033, I don't think we should make the jump just yet. I'm all for creating a separate branch where we can convert MUI so that it's compatible with what is proposed for 0.14 and make the components more functional (minimize/eliminate this.refs.XXX.YYY()).

elgerlambert commented 8 years ago

If the current use of this.ref.xxx doesn't actually break material-ui when used together with react 0.14.0-beta1 then I don't feel the wish to move away from that pattern should block #1033. By making it easier to install material-ui alongside react 0.14.0-beta1 you'll set yourself up to receive early feedback on actual issues that may arise; it's better to receive that feedback when 0.14 is still in beta.

Perhaps a good alternative (that better manages expectations), is to release an alpha/beta/rc version of material-ui on npm that has 0.14.0 as a peer dependency (and is geared towards making material-ui 0.14 compatible). That way it's easier for people to move forward and find/fix any issues that might exist.

ashtonsix commented 8 years ago

@jkruder Any update on this?

jkruder commented 8 years ago

@ashtonwar None yet -- I've been focusing on getting some testing established and resolving some external commitments. I should have some time this week to take a look at it and see what we're working with. Some others have already made some attempts at migrating to 0.14.

ashtonsix commented 8 years ago

Cheers for looking at this. I've seen material-ui-io from #1033. It seems to work for some components (dropdowns, buttons, snackbar) but fall over & die on others (checkbox, slider, toggle). Not aware of any other attempts to migrate.

oliviertassinari commented 8 years ago

@tleunen Turn out we can still use this.refs.XXX for custom component. Thank @jkruder .

jkruder commented 8 years ago

No problem; I still believe that it is best to avoid using this.refs.doSomething() where possible.

newtack commented 8 years ago

Any updates? React JS 0.14 RC 1 just got released and really would like to use Material-UI with it.

Gregoor commented 8 years ago

same here, is there a way we can support the migration to 14?

ashtonsix commented 8 years ago

I'm using material-ui-io in production - it seems OK.

juhaelee commented 8 years ago

So I'm the guy who published material-ui-io and it was a very rough and sad attempt at porting over material-ui.

I highly recommend you do not use this library in production! I made the port and published it in a day to test out material-ui, but ended up switching over to creating my own library on top of mdl

ghost commented 8 years ago

I'd say that the best way all of us can help with getting material-ui vetted on React 14 is to upgrade to React 14-rc1 and report the issues that arise individually. Starting the issue title with "React 14-rc1: this specific error happens...." or just letting the issue be tagged with an appropriate label is maybe a good idea.

But maybe not, in which case I hope the lead maintainers correct me

ghost commented 8 years ago

https://github.com/callemall/material-ui/pull/1647

Might need some touch up regarding peer-deps vs dev-deps vs deps and there is the issue with touch events that is pending.

kinolaev commented 8 years ago

In React 0.14 onTouchCancel, onTouchEnd, onTouchMove, onTouchStart work automatically, see https://facebook.github.io/react/blog/#breaking-changes. To enable onTouchTap without react-tap-event-plugin:

import EventPluginHub from 'react/lib/EventPluginHub';
import TapEventPlugin from 'react/lib/TapEventPlugin';
EventPluginHub.injection.injectEventPluginsByName({ TapEventPlugin });
oliviertassinari commented 8 years ago

Do we still have the 300ms delay of iOS Safari?

kinolaev commented 8 years ago

I don't have IOS... But in first post here https://github.com/facebook/react/issues/436, injecting TapEventPlugin is suggested as solution. Besides that here https://github.com/facebook/react/commit/ff12423d639413c1934dfc2ff337b298952e99ef i have found relevant commit.

tapas4java commented 8 years ago

Is there any tentative timeline for supporting React 14?? This issue is really old and it would be nice if it get resolved soon!

justjacksonn commented 8 years ago

I too was really excited to use this UI toolkit and avoid Bootstrap, Foundation and even Elemental UI... but I am using React 0.14 with Redux and am not going back to 0.13. How long before an update?

Also, not sure I qualify to ask/provide this info, but with regards to refs, (also not sure if this is a 0.14 thing.. or 0.12/0.13) I usually add this to my form input elements:

<input .... ref={(c) => this.name = c}/>

In my onclick (or whatever handler) code, I can access the value via this.name.value. Makes it very easy to get any of the input values. Any chance this is all that is needed to update the toolkit with working ref?

jwahyoung commented 8 years ago

+1 on this. Found material-ui today and was really excited to try it! Unfortunately, no dice.

rohitahuja commented 8 years ago

+1. Would also like to know the timeline of this update!

muloka commented 8 years ago

:+1:

shaurya947 commented 8 years ago

We're getting there, folks! See #1751. At this point a little more work is needed to upgrade to the new react-router api.

I would recommend trying out the react-0.14-support branch and reporting any issues with the [React0.14] prefix in the title. Once we can get that branch fully working I shall close this issue (finally!) :)

justjacksonn commented 8 years ago

Great to hear! Look forward to a final version. I have been working with Redux, React, react-router, and so far its a pretty nice way to go. Look forward to incorporating Material UI in to this.

On Tue, Sep 29, 2015 at 1:31 PM, Shaurya Arora notifications@github.com wrote:

We're getting there, folks! See #1751 https://github.com/callemall/material-ui/pull/1751. At this point a little more work is needed to upgrade to the new react-router api.

I would recommend trying out the react-0.14-support branch and reporting any issues with the [React0.14] prefix in the title

— Reply to this email directly or view it on GitHub https://github.com/callemall/material-ui/issues/1030#issuecomment-144183104 .

carlitux commented 8 years ago

Cool! I will be putting issues if there is anything...

amagdas commented 8 years ago

Any news on a release date for React 0.14 support?

shaurya947 commented 8 years ago

@amagdas are you aware of the react-0.14-support branch? It's an ongoing effort. Feel free to test it out and report any issues with the [React0.14] prefix

amagdas commented 8 years ago

@shaurya947 Yup, I'm aware of it, but I'm not able to install the branch using npm, trying again. Having some sort of Readme/wiki on how to test this branch using react 0.14 would be good.

clkao commented 8 years ago

you can either npm link from a clone or do npm i 'git://github.com/callemall/material-ui#react-0.14-support' in your project.

justjacksonn commented 8 years ago

To be clear you need to do the npm install in the node_modules directory not the root of your directory On Oct 2, 2015 08:01, "Chia-liang Kao" notifications@github.com wrote:

you can either npm link from a clone or do npm i 'git:// github.com/callemall/material-ui#react-0.14-support' in your project.

— Reply to this email directly or view it on GitHub https://github.com/callemall/material-ui/issues/1030#issuecomment-145051787 .

shaurya947 commented 8 years ago

@amagdas this branch is not up on npm yet as it still has some issues and is a work in progress.

You can either do what @clkao said, or, after cloning the repository on your machine, switch to the react-0.14-support branch using git checkout react-0.14-support.

After that when you run npm i in the root directory, all source files get compiled into the lib folder. You can then use this lib folder in your project.

amagdas commented 8 years ago

Yup, done this and it works, will give feedback.

newtack commented 8 years ago

How about leveraging this FB tool to automatically make the changes? https://github.com/facebook/react/blob/master/packages/react-codemod/README.md

kinolaev commented 8 years ago

Look at "Notable bug fixes" in react 0.14 announce (http://facebook.github.io/react/blog/2015/10/07/react-v0.14.html): "Click events are handled by React DOM more reliably in mobile browsers, particularly in Mobile Safari." ...

oliviertassinari commented 8 years ago

@kinolaev For more detail: https://github.com/callemall/material-ui/pull/1732#issuecomment-143478944

ovaris commented 8 years ago

Is react-0.14-support removed?

oliviertassinari commented 8 years ago

@ovaris I was merged to master.