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

[ButtonBase] Document how to use cursor not-allowed #17778

Merged

Conversation

mark-tate
Copy link
Contributor

@mark-tate mark-tate commented Oct 8, 2019

Fixes #14455

Proposal for supporting disabled cursor on Button without breaking links

@mui-pr-bot
Copy link

mui-pr-bot commented Oct 8, 2019

No bundle size changes comparing 2b9d278...c00a3f3

Generated by 🚫 dangerJS against c00a3f3

@oliviertassinari oliviertassinari added the component: ButtonBase The React component. label Oct 8, 2019
@oliviertassinari oliviertassinari changed the title [ButtonBase] fix disabled cursor for button [ButtonBase] Document how to use cursor not-allowed Oct 8, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 8, 2019

It seems that we have to make a tradeoff between #11601 and #14455. We can't have both at the same time. Given the number of upvotes, I think that we should prioritize for disabled button with tooltips. So, I have added a small documentation note. It's what I have tried to do with the last commit.

@oliviertassinari oliviertassinari force-pushed the fix/ButtonBase/fix-disabled-cursor branch from 3e054e3 to b995ed5 Compare October 8, 2019 09:22
@oliviertassinari
Copy link
Member

@slipmat Let me know what you think. Changing the DOM structure wasn't an option.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather advise wrapping a span around it and add the cursor there. Otherwise polyfilling pointer-events: none; is quite difficult.

https://codesandbox.io/s/material-ui-disabled-button-cursor-not-allowed-01i69

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 9, 2019

I would rather advise wrapping a span around it and add the cursor there. Otherwise polyfilling pointer-events: none; is quite difficult.

@eps1lon Oh great, we can explain this approach too 👍 . The use of "rather" confuses me. This seems to be a complementary solution. We should be able to leverage :disabled too. I'm updating the wording to take your feedback into account. Thanks

@eps1lon
Copy link
Member

eps1lon commented Oct 9, 2019

This seems to be a complementary solution.

No it's meant as an alternate. Your solution implies a polyfilled behavior of pointer-events: none.

Please be aware that "not allowed" and "disabled" are different concepts. The assumption that you can just mix these concepts is wrong.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 9, 2019

No it's meant as an alternate. Your solution implies a polyfilled behavior of pointer-events: none.

Agree, I have changed to wording to explicit that these are two different options.

Please be aware that "not allowed" and "disabled" are different concepts. The assumption that you can just mix these concepts is wrong.

I think that it depends of the cases, In this proposal:

  • Option 1 is the simplest possible approach that has more limitations. Its the 80/20 pareto solution where people can apply a global CSS style, it will assume not allowed and disabled are the same, it won't change the style of a link element, and will require manual labor with a disabled tooltip.
  • Option 2, the solution requires a wrapper element, it's more cumbersome to use (potentially require to reexport a new button component, and to change the DOM structure when disabled) but has fewer limitations.

@eps1lon
Copy link
Member

eps1lon commented Oct 9, 2019

Option 1 hides an actual bug in your UI though. That has nothing to do with some 80/20 use case but with how much you're ok with a broken UI. I really can't help anyone if a simple wrapper is already to much work to do. If that's your issue then why even go out of your way and add a different cursor. Work is ok up to N but N + 1 is all of the sudden not workable?

I think that it depends of the cases

What cases?

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 9, 2019

you're ok with a broken UI

What do you mean by broken UI?

I really can't help anyone if a simple wrapper is already to much work to do.

My concern is how would developers deploy the solution, at scale (in all their pages)? I have seen a push back from the developers any time creating a wrapper and reexporting it from our components is required. I would like to avoid it.

I believe we can ask @andyford. How did you solve the problem, in the end?

What cases?

What I mean is that some design systems mix the two, e.g. Blueprint.

@andyford
Copy link

andyford commented Oct 9, 2019

I believe we can ask @andyford. How did you solve the problem, in the end?

Hey, in the end our team decided to use a different UI library ... not because of this topic, but becuase we decided to go with a more bare-bones approach.

So unfortunately I never solved this

@oliviertassinari
Copy link
Member

@andyford Thanks for sharing. Can we learn more about the tradeoff your team took? I could help us in the future. What solution did you pick? What do you mean by bare-bones?

@andyford
Copy link

andyford commented Oct 9, 2019

In a general sense, we were deciding between 2 options:

  1. use a fully fleshed out UI library and spend our time learning it and overriding it to fit our brand styles
  2. write our UI components from scratch but aided by lighter weight libs like styled-system

we chose option 2 because our app isn't super huge and we predicted (whether correct or not) that we would be spending more time to learn a UI lib and override it with our specific brand styles than we would if we wrote our stuff kinda from scratch (but with small helper libs)

For our small public-facing app which requires specific branding, I think we made the right choice. However we would pick Material UI (or something similar) in a hearbeat for an internal admin app which didn't need any (or very much) custom styling.

EDIT: we don't have anything against Material UI (in fact we are quite impressed), but we felt it was more than we needed for our particular use-case

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 9, 2019

@andyford Awesome! We are trying to break out people's perception of "Material-UI" from "Material Design UI" to "material to build UI".

I'm super interested in learning about the other low-level tools you have used in the process. For instance, did you consider using some of our lower level helpers? Modal, Clickaway, Portal, useMediaQuery, TextareAutosize, system, and soon Autocomplete, Dropzone, etc.

Among these open issues, which one would you qualify as the most important?

Thanks.

docs/src/pages/components/buttons/buttons.md Outdated Show resolved Hide resolved
docs/src/pages/components/buttons/buttons.md Outdated Show resolved Hide resolved
docs/src/pages/components/buttons/buttons.md Outdated Show resolved Hide resolved
packages/material-ui/src/Tooltip/Tooltip.js Outdated Show resolved Hide resolved
@oliviertassinari
Copy link
Member

@slipmat Thanks

@mwskwong
Copy link
Contributor

mwskwong commented Oct 13, 2019

@andyford Awesome! We are trying to break out people's perception of "Material-UI" from "Material Design UI" to "material to build UI".

I'm super interested in learning about the other low-level tools you have used in the process. For instance, did you consider using some of our lower level helpers? Modal, Clickaway, Portal, useMediaQuery, TextareAutosize, system, and soon Autocomplete, Dropzone, etc.

Among these open issues, which one would you qualify as the most important?

Thanks.

I would say this one (i.e. usage of dynamic theme variants and colours)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: ButtonBase The React component. docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

disabled buttons have pointer-events: none which prevent overriding of cursor property
7 participants