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

[SpeedDialAction] pass through button color props to speed dial action button #12986

Conversation

itskibo
Copy link
Contributor

@itskibo itskibo commented Sep 24, 2018

Resolves #12830.

Changes:

  • removed hard-coded background color style, to correctly pass through the button color props
  • added documentation example for colored speed dial actions

I do believe the description in the docs can be better, but currently lacking any inspiration, so feel free to change that if you wish! 😄

@mbrookes
Copy link
Member

mbrookes commented Sep 24, 2018

@itskibo Rather than remove the default color, it would be good if you could update it to match the Material v2 spec:

image

It also adds a backdrop, but that's a subject for another PR.

You can still override the color with the button className prop.

We could think about adding a color prop to SpeedDialAction, but I'm not convinced it's necessary.

@oliviertassinari oliviertassinari added the component: slider This is the name of the generic UI component, not the React module! label Sep 24, 2018
@itskibo
Copy link
Contributor Author

itskibo commented Sep 25, 2018

@mbrookes I believe that overriding the color through the className props is counter-intuitive, if we can use the ButtonProps instead.

Unless people plan on using custom colors outside of their primary and secondary color, which I highly doubt, I believe that allowing the color from the ButtonProps to pass through correctly is the way to go.

Furthermore, I will quote what you said on the related issue:
We could add a color prop to SpeedDial and SpeedDialAction to support primary and secondary. Do you want to work on it?

There is no need to add a separate color prop when the ButtonProps can pass it through.

Regarding the backdrop: I see a backdrop behind the speed dial actions, so not sure what you mean by that. I assume you mean the same backdrop as when a dialog is open?

@mbrookes
Copy link
Member

I believe that allowing the color from the ButtonProps to pass through correctly is the way to go

Then you would need to disable the default color when this is supplied. I don't know, it feels kinda kludgy.

I assume you mean the same backdrop as when a dialog is open?

Yes, as in the screenshot.

@itskibo
Copy link
Contributor Author

itskibo commented Sep 26, 2018

Then you would need to disable the default color when this is supplied. I don't know, it feels kinda kludgy.

I believe the default color has been removed in my branch. Both the background color and the color are now based on the button component itself.

@mbrookes
Copy link
Member

I believe the default color has been removed in my branch.

#12986 (comment)

@mbrookes mbrookes added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Sep 26, 2018
@itskibo
Copy link
Contributor Author

itskibo commented Sep 26, 2018

#12986 (comment)

The specs on the speed dial actions are not directly defined, therefore I believe that we can follow the specs of the mini fab component, as they look like one and the same.

In doing so, we can allow the passing through of the color from the button props, and leaving the rest on whichever the developer pleases, whether it be from overriding the theme, or setting a className, or having it as the default of the button itself.

I see no reason to not implement this change, as this is following the current specs of the (mini) fab, aside from the backdrop.

@mbrookes
Copy link
Member

The contrast with the background is low now (slightly more so with this PR), it will be worse with a backdrop.

Let’s benchmark what others are doing.

@itskibo
Copy link
Contributor Author

itskibo commented Oct 1, 2018

Decided to see if I could find some implementations of the speed dial actions in other projects, and so far I found a few that allow/have custom colors for the speed dial actions:

kumar529/Ng2FabSpeedDial example - repo
ecodev/fab-speed-dial example - repo
yavski/fab-speed-dial repo (example inside)

Since the official specs don't mention anything about what color the action buttons should be, I see no reason to not implement this for the time being.

Sure, the contrast may become an issue for some, but they should be able to override the colors in the theme itself, right?

@eps1lon
Copy link
Member

eps1lon commented Oct 1, 2018

One issue is that the FABs have the wrong color by default. It should not be some "arbitrary" faded color but whatever color variant is chosen. Depending on the expected background users should either choose OnBackground or OnSurface (with a backdrop we should default to OnBackground). These are not implemented at the moment so it might be a good idea to do so (see https://material.io/design/color/the-color-system.html#color-theme-creation).

So I think it is correct to not hard-code the color via css but let users pass it via ButtonProps.

@mbrookes
Copy link
Member

mbrookes commented Oct 1, 2018

@itskibo When I said benchmark, I was thinking more of some of the mainstream Material Design libraries (MCW, Vuetify, Angular Material etc.)

@eps1lon

On[e] issue is that the FABs have the wrong color by default.

Not "wrong", so much as haven't been updated for Material V2 (they match the v1 spec).

@eps1lon
Copy link
Member

eps1lon commented Oct 1, 2018

On[e] issue is that the FABs have the wrong color by default.

Not "wrong", so much as haven't been updated for Material V2 (they match the v1 spec).

That's always easy to say for me to say since I've never worked closely with v1 and I never found a comprehensive changelog for the specification.

@itskibo
Copy link
Contributor Author

itskibo commented Oct 1, 2018

@itskibo When I said benchmark, I was thinking more of some of the mainstream Material UI libraries (MCW, Vuetify, Angular Material etc.)

Vuetify FAB has colored action buttons, and ecodev/fab-speed-dial uses the Angular Material library as its base.

I do agree with @eps1lon that the default colors should be changed to match the v2 spec, but that is not what this PR is for (or the issue related to this PR). This PR is solely for allowing colors from the button props to pass through correctly to the speed dial actions, not refactor it to the v2 specs. I believe that should be a separate issue, as it also adds a backdrop.

@mbrookes
Copy link
Member

mbrookes commented Oct 1, 2018

I never found a comprehensive changelog for the specification

Wouldn't that have been nice! As it is, we are left to discover what has changed through these sorts of conversations.

The difficulty we will have in updating the FAB relates in part to your color prop proposal. It seems that we should support both grey on white and white on black in addition to primary and secondary, (and possibly even an Avatar FAB). But which (black or white) should be the default, and what should the value for the color prop be for the other?

The other difficulty will be that changing the default color of the FAB is that it is a (visual) breaking change. I know @oliviertassinari has been okay with that in the past, but we have a more clearly defined strategy on breaking changes now, and this would be a less than subtle one.

@eps1lon eps1lon added component: speed dial This is the name of the generic UI component, not the React module! and removed component: slider This is the name of the generic UI component, not the React module! labels Oct 3, 2018
@mbrookes
Copy link
Member

mbrookes commented Oct 9, 2018

This PR is solely for allowing colors from the button props to pass through correctly to the speed dial actions

But in doing so removes the logic that is used to apply colors based on the color prop, which is a retrograde step.

@itskibo
Copy link
Contributor Author

itskibo commented Oct 10, 2018

But in doing so removes the logic that is used to apply colors based on the color prop, which is a retrograde step.

Okay I am legitimately confused right now.

Before this change, it was impossible to specify whether the SpeedDialAction component should have a primary or a secondary color, due to the background color being hard-coded, thus overwriting the colors from the Button component inside.

After this change, it is now possible to set the color to primary and secondary and have it actually pass through and apply the color (as you would expect). If the color is omitted, it defaults to the background color of the Button component.

There is no "removing" logic in this PR, as far as I can see, so I am not sure what you are getting at right now.

@mbrookes
Copy link
Member

mbrookes commented Feb 2, 2019

Huh, thought I replied this this way back when... anyway, yes, sorry, I "misspoke". I should have revisited the code before commenting.

"Removes the default FAB color rather than adding the logic for selecting colors via a color prop."

Either the default should be updated, or if that's a matter for another PR, it should be left as is in this one for the time being, which I believe dictates a different approach for this PR.

Since this PR targets master, and we've moved on to next branch lets close for now. Lets pick up the discussion on the related issue, and figure out the best solution.

@mbrookes mbrookes closed this Feb 2, 2019
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! PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants