-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
Use either one of click / tap handlers in menus #581
Conversation
@pomerantsev Yeh, I agree that using both onClick and onTouchTap doesn't make sense. I think part of the problem is the bad design on the menu item component and we should strive for a more DOM like structure see (#27). With that said, we need to do something to resolve this issue in the mean time. I prefer to just use onTouchTap, which is what you seem to be doing. However, the props are still named onItemClick? |
@hai-cea As I said, any name will do, but I feel that naming it something different from |
@pomerantsev What do you think about using react-tappable instead of react-tap-event-plugin? It seems to be better and we can just use onTap. |
@hai-cea Looks like react-tappable has a couple of nice abstractions (Press and Pinch events), but I'm not sure if using its Tap is any better than react-tap-event-plugin's TouchTap. It's just that it has a slightly different API, so instead of |
@hai-cea Should we get back to this discussion? Anything stopping you from introducing this breaking change for the sake of consistency? |
@pomerantsev Thanks for pushing this discussion. One thing I'm not sure about is if we should introduce a new synthetic event to a UI library. The original idea was to build our components as flexible as possible so that developers can choose which events they want to listen to - just like native elements. That is the reason behind exposing both onItemClick and onItemTap. With that said, it sounds like there are problems with this approach? |
@hai-cea Sorry, I guess I lost the point of this discussion :).
Looks like you're in favor of the first approach, whereas I like the second better. But I totally understand that it's not a question of my personal preference. Some examples from various components:
I guess I could come up with more examples, but you can see that the use of |
Thanks @pomerantsev - It was my fault for taking so long to get my thoughts together on this. I think some more research will definitely be helpful. The examples you have given so far are definitely short comings of the current components and I think we can address them on a case by case basis? Also, is your preference for the 2nd approach because of simplicity for developers? |
@hai-cea Okay, I guess you're right that this confusion is not the problem of material-ui, but rather of React and the fact that they introduced After looking more closely at the code, I see that there are several specific issues instead of a single big one - which makes them easier to deal with.
So, with all the aforementioned changes, I think this PR can be closed. I don't feel that there's a huge need to remove |
@pomerantsev Thank you very much for this! Yeh, I think we need to decide on whether we want to use onTap or onTouchTap. My preference is that we go with onTouchTap since the underlying event we're binding to is onTouchTap. What do you think? I've merged the other PRs you submitted and agree what we should create separate issues for #3 and #5. From what I understand from attending the React conference earlier this year, the react team is still working out how best to handle mobile. |
@hai-cea I created separate issues / PRs for everything listed above, so I'm closing this. |
Fixes #366.
This PR fixes a real issue, but even if @hai-cea and @mmrtnz don't like the implementation, I'd like to discuss whether it's a good idea to consistently use either onClick or onTouchTap in the API.
I'm not sure if it makes any sense to use both onClick and onTouchTap on the same element. onClick does the same thing as onTouchTap, only 300ms later on some devices.
So my proposal is to only use onTouchTap on native elements (since the project depends on react-tap-event-plugin anyway), and to use the Click idiom for all material-ui components, just because everyone's used to it.
Although using onClick directly on a component which internally transforms it into an onTouchTap is a bit confusing, so instead, I would prefer to have a property called onItemClick or something (see PR).
If you're happy with this approach, you could merge it, and I could proceed with the rest of the codebase.