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] Add a color prop to SpeedDial & SpeedDialAction #12830

Closed
1 task done
helfi92 opened this issue Sep 10, 2018 · 16 comments · Fixed by #17301
Closed
1 task done

[SpeedDial] Add a color prop to SpeedDial & SpeedDialAction #12830

helfi92 opened this issue Sep 10, 2018 · 16 comments · Fixed by #17301
Assignees
Labels
component: speed dial This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. new feature New feature or request

Comments

@helfi92
Copy link
Contributor

helfi92 commented Sep 10, 2018

I'm trying to change the color of the speed dial component via the ButtonProps prop.

  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

The speed dial component should change colors.

Current Behavior

The color doesn't change.

Steps to Reproduce

Link:

  1. https://codesandbox.io/s/9yvm1xy9pp

Your Environment

Details can be seen in the codesandbox link.

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

I'm looking at a purple speeddial in your codesandbox. What am I missing? 😄

@helfi92
Copy link
Contributor Author

helfi92 commented Sep 11, 2018

Sorry, I should have provided more information. It's with regards to the SpeedDialAction component. I expected

<SpeedDialAction
  ButtonProps={{ color: "secondary" }}
  icon={<FileCopyIcon />}
/>

to change the color of the SpeedDialAction button but it stayed the same.

@mbrookes
Copy link
Member

mbrookes commented Sep 11, 2018

Got it. The SpeedDialAction color is being set to match the spec (the Material v1 spec at least) via a JSS class:

  /* Styles applied to the `Button` component. */
  button: {
    margin: 8,
    color: theme.palette.text.secondary,
    backgroundColor: emphasize(theme.palette.background.default, 0.12),
    '&:hover': {
      backgroundColor: emphasize(theme.palette.background.default, 0.15),
    },
    transition: `${theme.transitions.create('transform', {
      duration: theme.transitions.duration.shorter,
    })}, opacity 0.8s`,
    opacity: 1,
  },

So to change the color, you would have to override the backgroundColor styles via the SpeedDialAction classes prop.

We could add a color prop to SpeedDial and SpeedDialAction to support primary and secondary. Do you want to work on it?

@mbrookes mbrookes changed the title [LAB][SpeedDialAction] Changing button color [SpeedDial] Add a color prop to SpeedDial & SpeedDialAction Sep 11, 2018
@mbrookes mbrookes added new feature New feature or request good first issue Great for first contributions. Enable to learn the contribution process. labels Sep 11, 2018
@helfi92
Copy link
Contributor Author

helfi92 commented Sep 11, 2018

The color prop sounds good to me. I'll send a PR.

@itskibo
Copy link
Contributor

itskibo commented Sep 24, 2018

Hi @helfi92, do you have a status update regarding this issue? We would like to use it in our project as well, but if you don't have the time for it, I am willing to pick it up.

@helfi92
Copy link
Contributor Author

helfi92 commented Sep 24, 2018

@itskibo Please feel free to take over. Many thanks for offering your help.

@eps1lon eps1lon added the component: speed dial This is the name of the generic UI component, not the React module! label Oct 3, 2018
@robrothedev
Copy link

robrothedev commented Oct 19, 2018

@itskibo @helfi92 For what it's worth I was able to change the SpeedDialAction icon color and background by wrapping the icon in a span with a className prop:

{Actions.map(action => {
  let icon = (
    <span className={classes.speedDialIcon}>{action.icon}</span>
  );
  return (
    <SpeedDialAction
      key={action.name}
      icon={icon}
      tooltipTitle={action.name}
      tooltipOpen
      onClick={this.handleClick}
    />
  );
})}

Working example: https://codesandbox.io/s/y3985wzn5z

@charitha
Copy link

charitha commented Jan 22, 2019

Hi @mbrookes,
I'm new to MUI, I've gone through contribution guidelines. Since this issue is tagged with the good first issue If no one has picked it up yet, I'd like to give this one a try or any other suggestions?.

@mbrookes
Copy link
Member

@charitha The future of the SpeedDial component is up in the air at the moment - it's the least used component, so we're debating whether to drop it (or at least transfer it to another maintainer) in order to concentrate on the higher value components, so I would hold off for now.

CC @oliviertassinari...

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 22, 2019

It's always the issue with a proof of concept or any experimentation. When do you know that you have poor enough resources to validate the assumption? @mbrookes Is right. I would like to kill this component. Basically, I would like to kill anything that has a low adoption rate and focus the energy on improving what people are already using or what people have a strong need for. I think that low used components are better off deferred to the community. I think that we should pour more energy into what we do great to make it awesome. Basically:

  • Provide a strong core.
  • Simplifications, simplifications, simplifications: fight entropy!

@itskibo
Copy link
Contributor

itskibo commented Jan 23, 2019

@mbrookes @oliviertassinari maybe the adoption of this component could be low due to the fact that it's still marked as a lab component?

My two cents regarding this:
Just because the adoption is low, doesn't mean that you should drop it. The functionality of it is fine as is, although it does need a good refactoring.
At work, we use this component in quite a few places in our application, because any other button component makes no sense and requires a vigorous rework of the layout (something which we will not do for the foreseeable future, as our current layout is fine).

I can imagine that I am not the only one who uses this component because it makes the most sense in certain layout contexts.

And let's say that you do make the decision to drop it, how would you suggest that we reimplement this functionality ourselves? Do you plan on writing up a migration document so we keep the same functionality with ease? Or do you plan on disappointing a part of your user base?

If I search for react speed dial in a search engine, the first link is a package built on top of the old FAB component (link), and the second link is to this component.

I believe that it is better to mark the current component as deprecated, and start working on a new one, rather than just dropping it altogether.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 23, 2019

@itskibo It's not the only component in the lab. We have 3 components. Take the last few weeks of GA stats. The speed dial has around 3k page views. The Toggle Button has 5k page views and the Slider 7k. Now, this has to be put into perspective with our TextField & Button components: 77k pages views each. The more components we support, the more energy we spend on low ROI components. It's slowing us down. I would rather cut the long tail to reinvest the energy in improving what's most used & doing more experimentations. I'm sure you are not the only one using this component. People should be able to take the code as is and host it somewhere else.

@itskibo
Copy link
Contributor

itskibo commented Jan 23, 2019

@oliviertassinari the view count between components in the core and components in the lab are in my eyes not comparable, because of course the majority of people will look under "Component Demos" or "Component API" rather than some ambiguous label named "Lab", which under the about section states that it "hosts the incubator components that are not yet ready to move to the core", making it seem like they are unstable and are subject to (breaking) change. This already explains the major difference between views, and are not related due to a low interest.

Furthermore, you can also see if someone in the community is willing to work on getting it ready to be moved to the core (which @charitha offered, and which I am willing to do as well in my free time next week), rather than dropping it or having it moved to another host package.

I don't see how dropping this component (and also solid support for it) and/or moving it to another host solves anything for the people who use it, because this is something that is defined in the spec, and I am quoting this directly from the website: "Once we implement the Material design specification (which is a bar set quite high), you should be able to take advantage of it for you own business with any style customization needed."

If you choose to drop it, or not find a solution for a replacement within this package, then you are not keeping to this statement you made.

As I see it, the new floating action button can perfectly fit the use case of the speed dial: https://material.io/design/components/buttons-floating-action-button.html#types-of-transitions

Implementing the speed dial functionality within the new floating action button component means that this component can be deprecated (and eventually removed), and it follows the guidelines of the floating action button as well.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 23, 2019

@itskibo Moving a component to the core or to a stable state has a strong implication. We guarantee the quality of the components we host in the core. Our users have the guarantee it will provide them a net ROI. They won't have to fight with crazy bugs, have to rewrite their code every month, etc. Now, to get to this stable state, it requires a lot of effort (time) from the maintainers, people like you who help and people like @helfi92 who report bugs. If I had to guess, I would say that we have only put 30% of what this component will require us in terms of time. We still have 70% to go.
So far, I think that its @mbrookes who has invested the more time in this component. So ultimately, it should be his call. As for me, I see so many areas that have a higher ROI, that I don't want to invest time in this component. Just for the lab, I would start with the Slider.

I don't see how dropping this component (and also solid support for it) and/or moving it to another host solves anything for the people who use it

It allows to scale the code ownership. Take the material-ui-pickers package as an example. I don't think that the author would have put that much amount of energy if the reward wasn't on his name. It comes down to how we manage the incentives.
As a user of Material-UI, I think that you can expect more output. Both because Material-UI maintainers can focus on the core components and because someone from the community can embody the mission of providing the best speed dial component.
The Vue.js community has been really good at executing this strategy.

@mbrookes
Copy link
Member

@itskibo @charitha As a compromise, what do you think about us merging PRs for lab components that have been reviewed and approved by another member of the community (and pass CI), without core team oversight?

It risks feature creep, and deviation from core code standards that could be an inhibitor to a component moving to core in the future, but it would allow them to stay under the mui-org umbrella.

@itskibo
Copy link
Contributor

itskibo commented Jan 31, 2019

@mbrookes seems like a compromise I'm willing to accept. But it might be a good idea to appoint someone of the community (or more than one person) who keeps a full oversight of the lab components, which I am willing to do in my spare time.

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! good first issue Great for first contributions. Enable to learn the contribution process. new feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants