fix(Button): render disabled button as span #1540

s0ber · 2020-09-01T20:34:11Z

Description

Right now we render disabled button as <button disabled /> with no pointer events. And this stands in our way when we want to render the tooltip (which is a very common case in Staff Portal). And we have to wrap the button in additional span just to render this tooltip. In many cases it breaks the styling (please take a look at this bug for example: https://toptal-core.atlassian.net/browse/SPT-880). There are also problems with button groups styling when one of the buttons is disabled.

The solution is simple. Let's not block all pointer events for disabled buttons and just render them as spans in the first place like we do in legacy platform.

Downsides: some tests may break in various projects and in general it's not always good when DOM element changes. But in the longterm it'll allow to avoid many hacks related to disabled buttons.

How to test

Open Button examples
Make sure disabled buttons are rendered as spans
Check out new Button example with even handlers

Review

Read CONTRIBUTING.md and Component API principles
Make sure you've converted all js/jsx file into ts/tsx in your PR
Annotate all props in component with documentation
Create examples for component
Ensure that deployed demo has expected results and good examples
Ensure that unit tests pass by running yarn test
Ensure that visuals tests pass by running yarn test:visual. If not - check the documentation how to fix visual tests

PR commands

List of available commands:

@toptal-bot run all - Run whole pipeline
@toptal-bot run danger - Danger checks
@toptal-bot run lint - Run linter
@toptal-bot run test - Run jest
@toptal-bot run build - Check build
@toptal-bot run test:visual or @toptal-bot run visual - Run visual tests
@toptal-bot run deploy:documentation - Deploy documentation

s0ber · 2020-09-01T20:37:30Z

@toptal-bot run package:alpha-release

s0ber · 2020-09-01T21:13:50Z

@toptal-bot run all

hweeTan · 2020-09-02T01:14:44Z

@s0ber Or maybe we can adjust the Tooltip component so it has a prop to always wraps the children with a Container or something? Because to me, it makes sense for disabled button not to have pointer-events. The issue here maybe is with Tooltip relying on the children to show, so maybe we should make it work regardless what the children are? WDYT?

ertrzyiks · 2020-09-02T06:44:07Z

packages/picasso/src/Button/Button.tsx

@@ -186,7 +187,7 @@ export const Button = forwardRef<HTMLButtonElement, Props>(function Button(
      title={title}
      value={value}
      type={type}
-      component={as!}
+      component={disabled && as === 'button' ? 'span' : as!}


We would need aria-role='button' to render a button with a span tag

Will add it, thanks!

@ertrzyiks It seems aria-role="button" is not valid, React complains to it. There is role="button" that is added by MUI, it seems that should be enough.

s0ber · 2020-09-02T07:32:20Z

@hweeTan Additional markup coming from the tooltip is not good, IMHO. It'll again bring unexpected styling issues. It's a leaking abstraction — you'll now have to account to the fact that everything can be wrapped into additional span. Basically it'll affect every possible component.

borisyordanov · 2020-09-02T07:38:50Z

packages/picasso/src/Button/story/index.jsx

    title: 'Disabled',
    description: 'The button shows that currently unable to be interacted with'
  })
+  .addExample('Button/story/DisabledWithEvents.example.tsx', {
+    title: 'Disabled with mouse events',
+    description: 'The button is disabled, but event handlers can be added'


This seems counter-intuive. If the button is disabled so should event handlers. If events are happening then the button is not disabled.

IMO we should just integrate a tooltip in the button.

<Button disabled disabledMessage="No bueno" // automatically renders a tooltip with the specified content />

Sounds good to me, but the problem is that with disabled button there won't be a way to add the tooltip. If we add the tooltip then the button stops being disabled (and any other event handler can be added).

@s0ber What if we set the tooltip as a child of the button?

@borisyordanov I just checked if events are fired for child nodes and it seems they are, at least in Chrome. The only problem I see here is that we'll need to change styles a bit. I'll try playing with this and prepare a complete solution to see how good will it look like in the end.

@borisyordanov I decided not to examine the option of adding the tooltip prop inside the Button because Tooltip is much more than just a single prop. And it doesn't bring many benefits really — just couples Button and Tooltip, also brings a lot of complicated logic inside the Button, requires significant styling refactoring, pollutes the markup with additional span for every button and so on 🙂
copied from Slack for the historical reasons

vedrani · 2020-09-02T15:02:19Z

Be aware it needs compatibility with v5 also. Check all variants here https://share.goabstract.com/f9b77f8f-544b-4ced-a6a3-76b5a32f2934?sha=2717bd241694d688a866dfab6dddc3f75edd671c

s0ber · 2020-09-02T22:46:46Z

@vedrani I'll help merging it into v5 branch!

borisyordanov · 2020-09-03T07:35:00Z

packages/picasso/src/Button/story/DisabledWithEvents.example.tsx

+import { Button } from '@toptal/picasso'
+import { useNotifications } from '@toptal/picasso/utils'
+
+const Example = () => {


I don't understand what value this example brings us. I'd rather see examples that cover

a disabled button that shows a tooltip on hover

a button group where one or more buttons are disabled and show a tooltip

Will add proper examples!

borisyordanov · 2020-09-03T07:35:16Z

packages/picasso/src/Button/story/index.jsx

@@ -188,10 +188,14 @@ page
  .addExample('Button/story/Basic.example.jsx', 'Basic')
  .addExample('Button/story/Variants.example.jsx', 'Variants')
  .addExample('Button/story/States.example.jsx', 'States')
-  .addExample('Button/story/Disabled.example.jsx', {
+  .addExample('Button/story/Disabled.example.tsx', {


vedrani · 2020-09-03T08:40:59Z

@vedrani I'll help merging it into v5 branch!

Please check #1537

borisyordanov · 2020-09-04T07:48:56Z

packages/picasso/src/Button/Button.tsx

@@ -84,7 +84,7 @@ export interface StaticProps {
 }

 const getVariantType = (variant: VariantType) => {
-  const [type] = variant!.split('-')
+  const [type] = variant!.split('-') // eslint-disable-line @typescript-eslint/no-non-null-assertion


is this eslint disable necessary?

For some reason precommit hook started complaining to me yesterday, maybe it started happening when Davinci dependency version was bumped recently, not sure TBH.

this is pretty strange, can you please figure out why this error started to happen?

borisyordanov · 2020-09-04T07:50:04Z

packages/picasso/src/Button/Button.tsx

      className={className}
      style={style}
      disabled={disabled}
      title={title}
      value={value}
      type={type}
-      component={as!}
+      component={disabled && as === 'button' ? 'span' : as!} // eslint-disable-line @typescript-eslint/no-non-null-assertion


Let's move these conditionals into variables?

borisyordanov · 2020-09-04T07:52:00Z

packages/picasso/src/Button/story/DisabledWithTooltip.example.tsx

+        </Tooltip>
+      </Container>
+
+      <Container top='small'>


Add some text above each of these examples so it's more obvious that the top example is two buttons and the bottom is a button group

borisyordanov · 2020-09-04T07:53:35Z

packages/shared/src/styles/withClasses/withClasses.tsx

+                  })
+                : configClasses
+                ? React.cloneElement(childNode, { classes: configClasses })
+                : childNode


@s0ber It'll take me a week and 100 espresso's to understand this code. Can we try to make it more readable? Maybe add some comments explaining what's going on

Yeap, it's tricky and I don't like it but I was not able to come up with something better. This code allows to inject classes and childrenClasses props into the chidren react nodes. Previously it was just classes. childrenClasses is a prop that you have to pass to Tooltip and this way they will be forwarded to the component that is wrapped by the Tooltip. We use this to pass classes from Button.Group to Tooltip to Button.

It technically may be useful in other cases where there are components that break the direct relationship between parent and children. E.g. previously there was just Button.Group -> Button relationship and Button.Group was passing some classes to Button for the proper styling. No when there is Tooltip between them things become more complicated.

I think we can proceed there and fix those things later because it is kind of internal technical issue, it only affects styling and it doesn't introduce anything in the public API.

toptal-devbot · 2020-09-04T17:30:34Z

🎉 Last commit is successfully deployed 🎉

Demo is available on:

https://picasso.toptal.net/disabled-button-as-span/

Your davinci-bot 🚀

denieler · 2020-09-07T07:10:58Z

isn't it a bad idea in terms of accessibility to render a button as span?

borisyordanov · 2020-09-07T07:23:38Z

isn't it a bad idea in terms of accessibility to render a button as span?

I guess it is, but considering we have been living with bad accessibility for over a year I think we can let this slide. I haven't seen any numbers, but I'm guessing the number of disabled Picasso users is low. When we start using Picasso for public pages we'll definitely have to revisit this.

denieler · 2020-09-07T07:35:16Z

I don't know the issue deep enough, but isn't there some better solution that using span instead of button? As I understand we have an issue that we need to wrap button with span to be able to show a Tooltip. But maybe we can fix Tooltip component instead to make it working with pure button tag

s0ber · 2020-09-07T07:48:41Z

@denieler If you have specific ideas I'll be very happy if you'll provide them. Maybes are not enough. I've posted multiple times why using button doesn't work in various places. There was actually a way to use it suggested by @borisyordanov — to point tooltip to the inner element of the button tag, but it was a bad solution from multiple points of view and it's explained in the PR comments.

As for ARIA, we use role="button" (it is actually being done by MUI):
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/button_role

denieler · 2020-09-07T07:54:00Z

I can provide solutions after closely checking possible options. I don't have any now, but I see we are fixing the consequences instead of the root of the problem

denieler · 2020-09-07T07:55:51Z

just an example, in MUI here they don't use any wrappers for Button component

s0ber · 2020-09-07T07:56:51Z

@denieler those buttons are not disabled

denieler · 2020-09-07T08:03:03Z

Looks like this is a suggested solution - mui/material-ui#8416 (comment)

You can add an additional div or span always as a wrapper for children prop of Tooltip

s0ber · 2020-09-07T08:04:55Z

@denieler please read the PR comments, this option was discussed

denieler · 2020-09-07T08:06:35Z

@denieler please read the PR comments, this option was discussed

if you can give me a link - it would be great

s0ber · 2020-09-07T08:07:31Z

@denieler
#1540 (comment)
#1540 (comment)

denieler · 2020-09-07T08:10:37Z

@hweeTan Additional markup coming from the tooltip is not good, IMHO. It'll again bring unexpected styling issues. It's a leaking abstraction — you'll now have to account to the fact that everything can be wrapped into additional span. Basically it'll affect every possible component.

I don't see an issue here. We can merge this as a breaking change into v5 branch. But instead, it will be a solid solution for a long time, instead of patching Button's disabled state.

s0ber · 2020-09-07T08:24:56Z

@denieler I'm not sure how random div or span in the markup can be a good solution considering it already breaks the styling in multiple places. After this change it'll affect every place where the Tooltip is being used — flex items will be broken, sibling selectors will be broken, table cells will be broken, nth-child selectors will be broken, etc.

denieler · 2020-09-07T08:27:42Z

@denieler I'm not sure how random div or span in the markup can be a good solution considering it already breaks the styling in multiple places. After this change it'll affect every place where the Tooltip is being used — flex items will be broken, sibling selectors will be broken, table cells will be broken, nth-child selectors will be broken, etc.

the breaking change is not an issue, as I said. It's just 1-time work. But we can ground a new way of using Tooltip if needed, a new behaviour of the Tooltip markdown and styles, if needed as well.

s0ber · 2020-09-07T08:30:43Z

@denieler It's not just a breaking change. You'll have to test every possible component and a combination of components and all new components that will ever appear just to figure out that additional span in the markup won't break the styling. It'll be something that is very hard to test but very easy to break.

denieler · 2020-09-07T08:34:52Z

@denieler It's not just a breaking change. You'll have to test every possible component and a combination of components and all new components that will ever appear just to figure out that additional span in the markup won't break the styling. It'll be something that is very hard to test but very easy to break.

yes, and this is true for every breaking change in Picasso. We shouldn't be afraid of making them still. Of course, they are unlikely, but sometimes we need to choose between 2 evils

s0ber requested a review from a team as a code owner September 1, 2020 20:34

s0ber force-pushed the disabled-button-as-span branch from bfe7c1e to 9d6f5ba Compare September 1, 2020 20:36

ertrzyiks reviewed Sep 2, 2020

View reviewed changes

borisyordanov reviewed Sep 2, 2020

View reviewed changes

s0ber added pending answer waits-for-answer labels Sep 2, 2020

borisyordanov reviewed Sep 3, 2020

View reviewed changes

s0ber force-pushed the disabled-button-as-span branch 3 times, most recently from 81d6c83 to 487e44c Compare September 3, 2020 22:09

borisyordanov reviewed Sep 4, 2020

View reviewed changes

s0ber force-pushed the disabled-button-as-span branch from 2ecba2b to 6e1496e Compare September 4, 2020 16:42

fix(Button): render disabled button as span

ce28ca2

s0ber force-pushed the disabled-button-as-span branch from 6e1496e to ce28ca2 Compare September 4, 2020 17:25

s0ber removed pending answer waits-for-answer labels Sep 4, 2020

borisyordanov approved these changes Sep 7, 2020

View reviewed changes

s0ber closed this Sep 7, 2020

denieler deleted the disabled-button-as-span branch December 18, 2020 09:27

fix(Button): render disabled button as span #1540

fix(Button): render disabled button as span #1540

Conversation

s0ber commented Sep 1, 2020 • edited by jira bot

Description

How to test

Review

s0ber commented Sep 1, 2020

s0ber commented Sep 1, 2020

hweeTan commented Sep 2, 2020

ertrzyiks Sep 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s0ber commented Sep 2, 2020

borisyordanov Sep 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vedrani commented Sep 2, 2020

s0ber commented Sep 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vedrani commented Sep 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s0ber Sep 4, 2020 • edited

Choose a reason for hiding this comment

toptal-devbot commented Sep 4, 2020

denieler commented Sep 7, 2020

borisyordanov commented Sep 7, 2020

denieler commented Sep 7, 2020

s0ber commented Sep 7, 2020 • edited

denieler commented Sep 7, 2020

denieler commented Sep 7, 2020

s0ber commented Sep 7, 2020

denieler commented Sep 7, 2020

s0ber commented Sep 7, 2020

denieler commented Sep 7, 2020

s0ber commented Sep 7, 2020

denieler commented Sep 7, 2020

s0ber commented Sep 7, 2020

denieler commented Sep 7, 2020

s0ber commented Sep 7, 2020

denieler commented Sep 7, 2020

s0ber commented Sep 1, 2020 •

edited by jira bot

ertrzyiks Sep 2, 2020 •

edited

borisyordanov Sep 2, 2020 •

edited

s0ber Sep 4, 2020 •

edited

s0ber commented Sep 7, 2020 •

edited