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

[SpeedDial] Allow a tooltip placement prop #12244

Merged
merged 2 commits into from
Aug 7, 2018

Conversation

seanchambo
Copy link
Contributor

@seanchambo seanchambo commented Jul 23, 2018

Allows adding a placement prop for the tooltip of the speed dial action.
I'm making my speed dial go horizontal which makes the tooltip positioning wrong

Closes #10895

@seanchambo seanchambo force-pushed the tooltip_placement_speed_dial_action branch 3 times, most recently from 017fcc0 to e45ece3 Compare July 23, 2018 03:48
mbrookes
mbrookes previously approved these changes Jul 23, 2018
@mbrookes
Copy link
Member

mbrookes commented Jul 23, 2018

@seanchambo What technique are you using for your horizontal speed-dial? There's an open issue for it (#10895)...

@seanchambo
Copy link
Contributor Author

I am just applying a class to the root and actions on the speed dial with flexDirection: 'row-reverse'

@oliviertassinari oliviertassinari added the package: lab Specific to @mui/lab label Jul 24, 2018
@mbrookes
Copy link
Member

mbrookes commented Jul 24, 2018

Would you like to submit a PR for that?

@seanchambo
Copy link
Contributor Author

Yeah I can add it to this PR if you want

@seanchambo
Copy link
Contributor Author

@mbrookes I've added the commit to allow the actions to be placed in any direction

@seanchambo seanchambo force-pushed the tooltip_placement_speed_dial_action branch 2 times, most recently from f4c06c0 to 1236ef1 Compare July 25, 2018 01:25
@mbrookes mbrookes dismissed their stale review July 25, 2018 15:38

Stale

@mbrookes
Copy link
Member

@seanchambo Great!

@oliviertassinari Any thoughts on the prop name for the direction the SpeedDial opens? actionsPlacement is a bit verbose.

@mbrookes
Copy link
Member

@seanchambo For want of anything better, let's go with direction?: 'up' | 'down' | 'left' | 'right';

It would also be good to add a demo to the docs with perhaps a radio to select the direction. You would also need to change the FAB position according to the selected direction.

@oliviertassinari
Copy link
Member

@mbrookes actionsPlacement sounds good to me 👍

@mbrookes
Copy link
Member

@oliviertassinari I'm leaning towards direction...

Copy link
Member

@mbrookes mbrookes left a comment

Choose a reason for hiding this comment

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

Review comments are based on the assumption we go with direction, so hold off until we agree on that.

@@ -15,12 +15,26 @@ export const styles = {
root: {
zIndex: 1050,
display: 'flex',
flexDirection: 'column-reverse', // Place the Actions above the FAB.
},
/* Styles when actions are placed to the top */
Copy link
Member

Choose a reason for hiding this comment

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

Please follow the conventions used for these docs comments in rest of the components:

/* Styles applied to the root and action container elements when direction="up" */

actionsTop: {
flexDirection: 'column-reverse',
},
/* Styles when actions are placed to the bottom */
Copy link
Member

Choose a reason for hiding this comment

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

etc.

return (
<div className={classNames(classes.root, classNameProp)} {...other}>
<div className={classNames(classes.root, classNameProp, actionsPlacementClass)} {...other}>
Copy link
Member

Choose a reason for hiding this comment

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

classNameProp goes last.

@@ -191,6 +217,10 @@ class SpeedDial extends React.Component {
}

SpeedDial.propTypes = {
/**
* The placement of floating actions buttons in relation to speed dial.
Copy link
Member

Choose a reason for hiding this comment

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

"The direction the actions open relative to the floating action button."

@@ -55,6 +55,7 @@ class SpeedDialAction extends React.Component {
onClick,
open,
tooltipTitle,
tooltipPosition,
Copy link
Member

Choose a reason for hiding this comment

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

tooltipPlacement? (Might as well keep it consistent with the Tooltip prop name).

@seanchambo
Copy link
Contributor Author

There is a problem that I think needs to be addressed before we continue down this path.

The placement of the FAB will not honour the absolute positioning. For instance, if the absolute positioning was the bottom right hand corner and the direction was down. The FAB would be moved up the page and the last action in the actions list would be positioned in the bottom right hand corner. This is because of the structure of the SpeedDial component. This structure would need to be changed in order for the absolute positioning to be placed on the FAB.

I'm not sure if this is the expected behavior but I wanted to point this out before we go ahead.

@mbrookes
Copy link
Member

@seanchambo Yes:

You would also need to change the FAB position according to the selected direction.

Unless I'm missing something, you wouldn't want to absolute position in the bottom right, and have the actions go down or right.

BTW Lets go with "direction" as the prop name.

@oliviertassinari
Copy link
Member

BTW Lets go with "direction" as the prop name.

By the way, there is a native property with this name.

@mbrookes
Copy link
Member

there is a native property with this name

There's the HTML dir attribute, and CSS direction style, but I didn't find a direction attribute when I checked. Am I missing something?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 26, 2018 via email

@seanchambo
Copy link
Contributor Author

@mbrookes Okay great, you are right those scenarios aren't very likely but was making sure that all cases didn't need to be accounted for.
Happy to move forward then and change the prop name to direction

@seanchambo seanchambo force-pushed the tooltip_placement_speed_dial_action branch from 1236ef1 to d0e59e8 Compare August 1, 2018 05:25
@seanchambo seanchambo force-pushed the tooltip_placement_speed_dial_action branch 2 times, most recently from a5775fe to 1ee2436 Compare August 1, 2018 05:51
@seanchambo
Copy link
Contributor Author

@mbrookes have updated the PR to take care of your comments

let me know what you think

# Conflicts:
#	packages/material-ui-lab/src/SpeedDial/SpeedDial.js
#	pages/lab/api/speed-dial.md
@seanchambo seanchambo force-pushed the tooltip_placement_speed_dial_action branch from 1ee2436 to 1a12449 Compare August 1, 2018 23:51
Copy link
Member

@mbrookes mbrookes left a comment

Choose a reason for hiding this comment

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

@seanchambo Sorry for the delay, I was working on something else. LGTM.

@seanchambo
Copy link
Contributor Author

No worries, we happy to merge?

@mbrookes
Copy link
Member

mbrookes commented Aug 7, 2018

@oliviertassinari any objection?

@oliviertassinari
Copy link
Member

@mbrookes No objection :)

@mbrookes mbrookes merged commit d561bce into mui:master Aug 7, 2018
@mbrookes
Copy link
Member

mbrookes commented Aug 7, 2018

@seanchambo Thanks!

eps1lon added a commit to eps1lon/material-ui that referenced this pull request Aug 15, 2018
During review mui#12244 the names for directions changed and CI did
not run which introduced errors.
oliviertassinari pushed a commit that referenced this pull request Aug 15, 2018
During review #12244 the names for directions changed and CI did
not run which introduced errors.
@mbrookes mbrookes added new feature New feature or request component: speed dial This is the name of the generic UI component, not the React module! labels Sep 13, 2018
@oliviertassinari oliviertassinari removed the package: lab Specific to @mui/lab label Jan 9, 2021
@oliviertassinari oliviertassinari changed the title [SpeedDialAction] Allow a tooltip placement prop [SpeedDial] Allow a tooltip placement prop Jan 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: speed dial This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants