[SpeedDial] Rework part of the logic #17301

hashwin · 2019-09-03T17:31:37Z

Reimplementing #12870 with the updated material-ui.
This updates persistent tooltip styles to match the material design guide (https://material.io/components/buttons-floating-action-button/#types-of-transitions).

For people updating their code from @material-ui/lab@4.0.0-alpha.26 to alpha.27, the following diff should be enough:

      <SpeedDial
        ariaLabel="SpeedDial tooltip example"
        className={classes.speedDial}
        hidden={hidden}
        icon={<SpeedDialIcon />}
-       onBlur={handleClose}
-       onClick={handleClick}
+       onClose={handleClose}
-       onFocus={handleOpen}
-       onMouseEnter={handleOpen}
-       onMouseLeave={handleClose}
+       onOpen={handleOpen}
        open={open}
      >

Closes Speed Dial throws in MUI demos #17293: Fix re-render ref logic
Closes Opening of Dialog through SpeedDial on mobile does not work #16834: Add onClose and onOpen props, with tooltip-inspired event logic.
Closes [Speed Dial] tooltip moves while scrolling background #13908: Avoid using the tooltip component
Closes [Speed Dial] When you "wiggle" your mouse above Speed Dial, the tooltips lose alignment #13839: Avoid using the tooltip component
Closes [SpeedDial] Interesting keyboard behavior #13613: Keyboard behavior seems correct
Closes SpeedDialAction requires undocumented onClick handler on touch screens #13522: Add onClose and onOpen props, with tooltip-inspired event logic.
Closes RFC: SpeedDial and Tooltip vs. Labels #13088: Avoid using the tooltip component. I believe that we can leverage the best of the two worlds, the tooltip component is great for handling the trigger logic, based on interaction. The "static" approach is great when the labels have to be always visible, when no interactions are required.
Closes [SpeedDial] SpeedDialAction onClick not working when you click the touch screen #13006: Add onClose and onOpen props, with tooltip-inspired event logic.
Closes [SpeedDial] Speed dial action binding enter event #12862: Add onClose and onOpen props, with tooltip-inspired event logic.
Closes [SpeedDial] Add a color prop to SpeedDial & SpeedDialAction #12830: 1 year with no interest, it's probably not important, closing
Closes SpeedDialAction onClick broken for some touch screen devices #12646: Add onClose and onOpen props, with tooltip-inspired event logic.
Closes [SpeedDial] blur fired before click, breaks FF Mobile #11811: Add onClose and onOpen props, with tooltip-inspired event logic.
Closes [SpeedDial] SpeedDialAction visibility is poor on dark themes #10763: Make sure the contrast is correct with the dark mode.

mui-pr-bot · 2019-09-03T17:40:46Z

@material-ui/lab: parsed: +0.78% , gzip: +0.34%

Details of bundle changes.

Comparing: 0ddd56f...81a92f8

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	+0.03% 🔺	+0.05% 🔺	331,664	331,756	90,529	90,570
@material-ui/core/Paper	+0.02% 🔺	+0.04% 🔺	68,644	68,659	20,449	20,458
@material-ui/core/Paper.esm	+0.02% 🔺	+0.06% 🔺	62,083	62,098	19,192	19,203
@material-ui/core/Popper	0.00%	0.00%	28,397	28,397	10,159	10,159
@material-ui/core/Textarea	0.00%	0.00%	5,082	5,082	2,132	2,132
@material-ui/core/TrapFocus	0.00%	0.00%	3,766	3,766	1,596	1,596
@material-ui/core/styles/createMuiTheme	+0.09% 🔺	+0.17% 🔺	16,333	16,348	5,808	5,818
@material-ui/core/useMediaQuery	0.00%	0.00%	2,488	2,488	1,050	1,050
@material-ui/lab	+0.78% 🔺	+0.34% 🔺	153,589	154,786	46,739	46,896
@material-ui/styles	0.00%	0.00%	51,420	51,420	15,288	15,288
@material-ui/system	0.00%	0.00%	15,581	15,581	4,351	4,351
Button	+0.02% 🔺	+0.04% 🔺	78,595	78,610	24,014	24,024
Modal	+0.10% 🔺	+0.24% 🔺	14,527	14,542	5,102	5,114
Portal	0.00%	0.00%	2,907	2,907	1,318	1,318
Rating	+0.02% 🔺	+0.05% 🔺	69,948	69,963	21,844	21,856
Slider	+0.02% 🔺	+0.05% 🔺	75,069	75,084	23,173	23,184
colorManipulator	0.00%	0.00%	3,835	3,835	1,519	1,519
docs.landing	0.00%	0.00%	52,232	52,232	13,768	13,768
docs.main	+0.01% 🔺	+0.03% 🔺	597,147	597,233	190,633	190,685
packages/material-ui/build/umd/material-ui.production.min.js	+0.02% 🔺	+0.05% 🔺	302,599	302,665	86,944	86,984

Generated by 🚫 dangerJS against 81a92f8

oliviertassinari

Thanks for the effort.

packages/material-ui-lab/src/SpeedDial/SpeedDial.js

packages/material-ui-lab/src/SpeedDialAction/SpeedDialAction.js

oliviertassinari

Alright, it should be ready for review. I have done a few aggressive changes, I hope it's fine.

mbrookes · 2019-09-10T12:00:55Z

A couple of observations from playing with it (with no reference to the code):

When you tab onto the Fab the speed-dial opens, and you can then use the cursor keys to navigate the items. If the SpeeDial is closed (escape or space keys) key or by triggering an item (enter or space keys on an item), the cursor keys still navigate the items, but with incorrect focus appearance:

(sorry about the big image, retina macbook scaling issue)

In the case of preessing escape when an item is focussed, focus doesn't return to the Fab, and the behaviour is as above.

Also, when the speed-dial is horizontal, should the up-down arrow keys still work? (How does a blind keyboard user know the orientation of a pop-up?)

oliviertassinari · 2019-09-10T20:20:52Z

@mbrookes Thanks for the functionnal review, I have tried to solve all the concerns. However, I have no idea how to solve the incorrect tooltip focus display.

mbrookes · 2019-09-10T20:33:45Z

I have no idea how to solve the incorrect tooltip focus display.

Let's leave it for now then. That's still a huge number of issues resolved. Nice work guys!

mbrookes · 2019-09-11T12:32:13Z

docs/src/pages/components/speed-dial/OpenIconSpeedDial.js

        open={open}
      >
        {actions.map(action => (
          <SpeedDialAction
            key={action.name}
            icon={action.icon}
            tooltipTitle={action.name}
-            onClick={handleClick}
+            onClick={handleClose}


Might it be more helpful to have a click handler that shows how to identify which action has been clicked, before it calls onClose?

It could help, it would make the demo a bit more verbose, but could teach people a valuable pattern.

From (past) personal experience, it's the kind of thing React beginners struggle with. I think it might be worth a little verbosity.

Yes, I think that people can leverage index based handler when displaying data from the server, for instance, in the case of inbox (R.I.P.), the most frequent contacts. Or in the case of Google Calendar, I would expect each action to have a named custom handler.

I'm not sure how to action this comment. I would expect most people to have a fixed set of options (not using an array). However, our demo is using an array for the sake of minimizing the demo length. I believe these two objectives contradict themselves.

I think the point is that the click handler does not handle each action differently which should be the most common use case. Otherwise why have a speed dial if every action does the same (which only closes at the moment). I would expect that you want to dispatch some action. It would be nice if this demo would handle that case.

At least this is how I understood @mbrookes

Ok thanks, this sounds good 👍

@eps1lon Yeah, that's the crux of it. I took it from @oliviertassinari's comment that in real usage you wouldn't use a mapped array, but would compose each action component. If each then calls a different click handler, the issue goes away.

I had pictured a common handler that dispatches a different event according to the calling action.

Just use an inline handler that captures the action. A mapped array is the first iteration you would do. The rest is premature optimization.

I'm going to merge, I think that we can apply the handler changes in another pull request :).

dankolesnikov · 2019-09-28T05:52:54Z

Thank you for your work, everyone! SpeedDial is very helpful to us.

lsnch · 2019-10-01T13:17:43Z

Is there now a way to disable open/close on hover?

oliviertassinari · 2019-10-01T13:31:25Z

@lsnch Right now, you can use the first event argument and look at event.type. You can ignore mouse enter events.

lsnch · 2019-10-01T14:52:49Z

It seems there's something async going on, maybe the callback gets delayed for the sake of animation.
Here it says I'm trying to access synthetic event too late.
https://codesandbox.io/s/material-demo-5dptv

oliviertassinari · 2019-10-01T15:03:03Z

@lsnch Oh, thanks for reporting it. Yes, there is an async logic to debounce events. In this case, I would propose the following:

We add an event.persist() call, in case somebody wants to play with the event, we already do it for the Tooltip. I think that point is safe to move forward. ✅
We add a reason second argument to the onOpen / onClose callbacks, it's a pattern we already use in the case base. Possible values could be: escapeKeyDown, focus, blur, toggle, mouseEnter, and mouseLeave? 🤷‍♂️

A proposal to double-check with @mbrookes. If we agree, would you be intersted in submitting a pull request? :)

mbrookes · 2019-10-01T15:12:19Z

Sounds good.

lsnch · 2019-10-01T15:24:04Z

Yeah, absolutely, I'll give it a go.

oliviertassinari · 2019-10-07T08:31:52Z

@lsnch Will you have some time in the next few days? :)

lsnch · 2019-10-08T07:02:11Z

Yeah, sorry I took so long, didn't realize it was such a minor change.

acroyear · 2019-11-29T16:36:34Z

Thanks for all of this work. I'd stopped using SpeedDial a while ago due to it not working right on touch-screen desktops. Gave it a try today and it is working wonderfully.

oliviertassinari · 2019-11-29T16:38:19Z

@acroyear We are happy to hear it :D.

oliviertassinari added the component: speed dial This is the name of the generic UI component, not the React module! label Sep 3, 2019

oliviertassinari changed the title ~~Fix persistent tooltip labels~~ [SpeedDial] Fix persistent tooltip labels Sep 9, 2019

oliviertassinari reviewed Sep 9, 2019

View reviewed changes

oliviertassinari added the design: material This is about Material Design, please involve a visual or UX designer in the process label Sep 9, 2019

oliviertassinari self-assigned this Sep 9, 2019

oliviertassinari changed the title ~~[SpeedDial] Fix persistent tooltip labels~~ [SpeedDial] Rework part of the logic Sep 10, 2019

oliviertassinari force-pushed the hashwin/persistent_tooltip branch 2 times, most recently from 59b8e6d to c8c5e62 Compare September 10, 2019 02:17

oliviertassinari removed their assignment Sep 10, 2019

oliviertassinari requested a review from mbrookes September 10, 2019 02:17

oliviertassinari force-pushed the hashwin/persistent_tooltip branch 5 times, most recently from 2523221 to ee1e6a0 Compare September 10, 2019 11:18

oliviertassinari approved these changes Sep 10, 2019

View reviewed changes

oliviertassinari force-pushed the hashwin/persistent_tooltip branch from 015f714 to 36e54c0 Compare September 10, 2019 20:19

mbrookes reviewed Sep 11, 2019

View reviewed changes

oliviertassinari added the PR: ready to ship label Sep 13, 2019

hashwin and others added 5 commits September 14, 2019 11:20

Fix persistent tooltip labels

c100e82

Fix style

500db07

Update styles

ad0c626

Fix style

75a6ef4

push further

aa11abd

oliviertassinari force-pushed the hashwin/persistent_tooltip branch from 36e54c0 to ba11e84 Compare September 14, 2019 09:23

Matt review

81a92f8

oliviertassinari force-pushed the hashwin/persistent_tooltip branch from ba11e84 to 81a92f8 Compare September 14, 2019 09:36

oliviertassinari merged commit e9f26fc into mui:master Sep 15, 2019

lsnch mentioned this pull request Oct 8, 2019

[SpeedDial] Pass event and reason to onOpen, onClose #17783

Merged

oliviertassinari mentioned this pull request Mar 13, 2021

[SpeedDial] Open/Close synchronous #25320

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SpeedDial] Rework part of the logic #17301

[SpeedDial] Rework part of the logic #17301

hashwin commented Sep 3, 2019 •

edited by oliviertassinari

Loading

mui-pr-bot commented Sep 3, 2019 •

edited

Loading

oliviertassinari left a comment

oliviertassinari left a comment

mbrookes commented Sep 10, 2019

oliviertassinari commented Sep 10, 2019 •

edited

Loading

mbrookes commented Sep 10, 2019

mbrookes Sep 11, 2019

oliviertassinari Sep 11, 2019 •

edited

Loading

mbrookes Sep 11, 2019 •

edited

Loading

oliviertassinari Sep 11, 2019 •

edited

Loading

oliviertassinari Sep 13, 2019

eps1lon Sep 13, 2019

oliviertassinari Sep 13, 2019

mbrookes Sep 13, 2019

eps1lon Sep 13, 2019

oliviertassinari Sep 15, 2019

dankolesnikov commented Sep 28, 2019

lsnch commented Oct 1, 2019

oliviertassinari commented Oct 1, 2019

lsnch commented Oct 1, 2019

oliviertassinari commented Oct 1, 2019 •

edited

Loading

mbrookes commented Oct 1, 2019

lsnch commented Oct 1, 2019

oliviertassinari commented Oct 7, 2019

lsnch commented Oct 8, 2019

acroyear commented Nov 29, 2019

oliviertassinari commented Nov 29, 2019

[SpeedDial] Rework part of the logic #17301

[SpeedDial] Rework part of the logic #17301

Conversation

hashwin commented Sep 3, 2019 • edited by oliviertassinari Loading

mui-pr-bot commented Sep 3, 2019 • edited Loading

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari left a comment

Choose a reason for hiding this comment

mbrookes commented Sep 10, 2019

oliviertassinari commented Sep 10, 2019 • edited Loading

mbrookes commented Sep 10, 2019

mbrookes Sep 11, 2019

Choose a reason for hiding this comment

oliviertassinari Sep 11, 2019 • edited Loading

Choose a reason for hiding this comment

mbrookes Sep 11, 2019 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Sep 11, 2019 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Sep 13, 2019

Choose a reason for hiding this comment

eps1lon Sep 13, 2019

Choose a reason for hiding this comment

oliviertassinari Sep 13, 2019

Choose a reason for hiding this comment

mbrookes Sep 13, 2019

Choose a reason for hiding this comment

eps1lon Sep 13, 2019

Choose a reason for hiding this comment

oliviertassinari Sep 15, 2019

Choose a reason for hiding this comment

dankolesnikov commented Sep 28, 2019

lsnch commented Oct 1, 2019

oliviertassinari commented Oct 1, 2019

lsnch commented Oct 1, 2019

oliviertassinari commented Oct 1, 2019 • edited Loading

mbrookes commented Oct 1, 2019

lsnch commented Oct 1, 2019

oliviertassinari commented Oct 7, 2019

lsnch commented Oct 8, 2019

acroyear commented Nov 29, 2019

oliviertassinari commented Nov 29, 2019

hashwin commented Sep 3, 2019 •

edited by oliviertassinari

Loading

mui-pr-bot commented Sep 3, 2019 •

edited

Loading

oliviertassinari commented Sep 10, 2019 •

edited

Loading

oliviertassinari Sep 11, 2019 •

edited

Loading

mbrookes Sep 11, 2019 •

edited

Loading

oliviertassinari Sep 11, 2019 •

edited

Loading

oliviertassinari commented Oct 1, 2019 •

edited

Loading