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

fix: Allow to change background for FAB Group #758

Merged
merged 3 commits into from
Jan 28, 2019

Conversation

gawrysiak
Copy link
Collaborator

Motivation

It's currently possible to change the background color for FAB and also items within FAB.Group components. Changing the background color in FAB Group causes the overlay background to change.

This PR makes it possible to change the background for the FAB Group main button.

Current behavior:
screen shot 2019-01-25 at 17 41 56

Test plan

https://snack.expo.io/ryjNEaO7E

@callstack-bot
Copy link

callstack-bot commented Jan 25, 2019

Hey @gawrysiak, thank you for your pull request 🤗. The documentation from this branch can be viewed here. Please remember to update Typescript types if you changed API.

Copy link
Collaborator

@raajnadar raajnadar left a comment

Choose a reason for hiding this comment

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

LGTM ✌️

Copy link
Member

@satya164 satya164 left a comment

Choose a reason for hiding this comment

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

I think passing the background color to fab while passing rest of the styles to the container will be confusing (it's also a breaking change). There are few options:

  1. Pass whole style to FAB instead: this will be a breaking change
  2. Add a fabStyle prop
  3. Don't add a prop (users can still use theme.colors.accent to customize the FAB color)

I don't like adding props, but I'm leaning towards 2 because it's not a breaking change and is consistent with how you can customize the action buttons.

@gawrysiak
Copy link
Collaborator Author

Updated the PR to add an extra prop.

@satya164
Copy link
Member

Can you also update the typescript declaration files too?

@gawrysiak
Copy link
Collaborator Author

Ah, sorry, missed that. It's now added.

@satya164 satya164 merged commit b799a1e into callstack:master Jan 28, 2019
@gawrysiak gawrysiak deleted the fix/fabgroup-backgroundcolor branch January 28, 2019 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants