-
-
Notifications
You must be signed in to change notification settings - Fork 32k
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
[Core] Injects react-tap-event-plugin when calling getMuiTheme() #3079
Conversation
Resolves mui#3068 Signed-off-by: Neil Gabbadon <neil.gabbadon@emikra.com>
e920add
to
89d11e6
Compare
We should remove it from the examples too. |
Signed-off-by: Neil Gabbadon <neil.gabbadon@emikra.com>
Done. Do you think we should remove the section in the documentation about this? Or should we leave the section in and just change the content to say that material-ui now injects it for you. |
Looks like it's going to be an implementation detail of Material UI. I would say we can remote it from the surface of the |
I think we should reword the documentation to reflect the change, for the benefit of anyone who implemented the previous instructions. We can remove it completely in a future release. Edit: Obviously it's harmless if someone's already importing and injecting it in their app, but it might be a good idea to mention that we now do this in Material UI, and it no longer has to be added, and can optionally be removed if not used elsewhere in their app. |
I suppose the other option is to include the change log on the site and put a note on there. |
…efined in theme Signed-off-by: Neil Gabbadon <neil.gabbadon@emikra.com>
Signed-off-by: Neil Gabbadon <neil.gabbadon@emikra.com>
@oliviertassinari I'm not sure if you've been following the discussion in #3068, but there I figured if we wanted to pursue this PR, we probably should support overriding or disabling our call of /* custom muiTheme */
import injectTapEventPlugin from 'react-tap-event-plugin'
import Colors from 'material-ui/lib/styles/colors';
/* https://github.com/zilverline/react-tap-event-plugin#ignoring-ghost-clicks */
const customConfig = {
shouldRejectClick: function (lastTouchEventTimestamp, clickEventTimestamp) {
return true;
},
};
export default {
injectTapEventPlugin: () => injectTapEventPlugin(customConfig),
//injectTapEventPlugin: false, // This would turn off our call `injectTapEventPlugin` entirely
palette: {
primary1Color: Colors.cyan500,
primary2Color: Colors.cyan700,
},
}; This is the best idea I can think of at the moment that simplifies material-ui for the 80% of developers that aren't configuring react-tap-event-plugin and are new to material-ui but won't break for developers that want to do things their own way. @mbrookes @alitaheri Your feedback here would be useful too. 😄 |
I thought about this some more and I'm not a huge fan of this implementation. I don't have a better idea at the moment but I'll think about it. |
I myself would still go with the warning. This coupling can be harmful in ways we might not pick up so fast. after all, we are adding a plugin to react during a render, which scares me. |
It's not ideal, but I think it's worth the tradeoff. I propose we put it in a release candidate, and see if anyone screams. Would love to get @s0meone's thoughts on this. |
Yeah I'm not really completely comfortable with this either for that same exact reason. For the possible issues we're not aware of yet.
I'd be surprised if anyone screams during RC phase. But like @alitaheri said I'm afraid that down the line this could end up being a problem area. I think a fair compromise would be to merge the warning PR and see if we still are getting a lot issues opened up related to this issue. If after cleaning the code more we feel like we can reasonability fit this level of automation in and it's still an issue for users, I'd be open to reinvestigating it. |
Hi all, like I discussed with @oliviertassinari in zilverline/react-tap-event-plugin#57, I'm willing to add a mechanism to the plugin to detect whether the plugin is loaded or not. If people want to use the custom options provided with zilverline/react-tap-event-plugin#25 they could do so by injecting the plugin themselves, if not the proposed solution in this PR will inject the plugin automatically. Hopefully this tap plugin will be fixed by React and not needed anymore, but it doesn't seem they're actively working on this yet. So let know what an ideal solution would be and I'm happy to help out 👍 |
@s0meone Thanks for dropping a comment here! Yeah it seems like this event will be in user space indefinitely. I'm still thinking this through a bit and I'll let you know what I think the best direction is. Also FYI, I wrote a function that detects whether the plugin was loaded and warns if not here. Do you think that implementation is adequate? If detecting whether the plugin was loaded is something you think would have broader appeal to users of react-tap-event-plugin, then it'd be great to see it there. But I wouldn't ask you to do it if it's something only this project needs 😄 |
Your solution looks solid. I think this check is definitely useful for the plugin itself, we're getting several issue reports about this as well. Maybe we can get @madjam002 in here, would like to know what he thinks. |
Now that 0.14 has removed The only problem I see is if you want to use config like For now I can make it so calls to tap event plugin get ignored if it has already been injected? |
@madjam002 Thanks for the message and I apologize for the late response. 😄 First of all, I'm not too familiar with the internals of react-tap-event-plugin. I reviewed its source while implement the warning but I haven't looked too much beyond that. So please correct me if I'm wrong somewhere!
I agree that, ideally, multiple calls of Based on what I currently understand, the issue in zilverline/react-tap-event-plugin#59 is that since every call of Let me know if I'm reading that wrong. Honestly, I'm really conflicted about what the right solution is to this. Implementing a check that ignores successive calls to |
I suppose this is a potential scenario I'm afraid of:
Is this a reasonable concern or would this not happen? My gut feeling at the moment is that due to the global nature of the plugin, frameworks and libraries should avoid calling Side note: |
@newoga It's fine we can discuss it here if it's more convenient for you guys, material-ui is probably the biggest library which uses tap event plugin so it's important that we get this right.
Yeah that's what I'm worried about. To be honest, I don't mind calling Maybe we just need to improve error reporting so that if someone calls it twice, they no longer get a cryptic error message from React, but an error explaining that How does that sound? |
Yeah I was wondering the same. I think that's a real good start. I really think good error and/or warning messages will do a lot of good for the community that uses the plugin 👍 //cc @oliviertassinari @alitaheri @mbrookes Feel free to weigh in any additional thoughts here. |
Alright I will see to it later today 😄 |
Perfect, thanks much @madjam002! |
Sounds good guys. I think keeping things the way they are with better and clear warnings from both ends is a great solution for now. |
@madjam002 @s0meone Thanks for taking the time to discuss it with us 😄
I believe this is the best approach myself. I was thinking, maybe it won't be a bad idea if the same people who maintain
That's my opinion. I would love to hear yours on this too 😁 |
@alitaheri I like that idea. Maybe one call to |
Yeah, that's also a great idea 👍 An optional warning would also be good, so authors can point users to their section of documentation on how to set it up. some libraries rely on some of the configuration provided by |
@alitaheri That sounds reasonable 👍
That workflow should work too. @madjam002 Maybe you could get away with a monorepo approach like React and just stick both packages in the same repo and version them the same. |
👍 👍 |
👍 |
I like the sound of that, I'll see to it later this evening when I have some time. |
I'm going to close this PR because we're not going to support injecting tapEventPlugin automatically for developers. Thanks for everyone involved! |
Resolves #3068
Signed-off-by: Neil Gabbadon neil.gabbadon@emikra.com