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

Enable title coloring #101

Merged
merged 9 commits into from
Jan 31, 2019
Merged

Conversation

thomasw
Copy link
Contributor

@thomasw thomasw commented Dec 17, 2018

Related #47

This enables changing the snackbar title color on Android, which is likely a requirement if your app's theme defines a dark value for textColor (doing so overrides the default white color of the title and sets its color to textColor).

This pull request does not yet include the equivalent changes for iOS, which I'm unable to work on and verify at the moment. I'm opening this for visibility so someone can take it the rest of the way if they come across it before I have the chance.

The $FlowFixMe for processColor is no longer necessary because it is now properly typed in react-native. Additionally, disabling the no-param-reassign eslint rule is easily avoided, so this resolves that as well.

This shouldn't result in any behavioral changes.
The `color` option is processed the same way as the other color params and is then passed down to the native implementations where it can be used to style the snackbar's title text.
getView and findViewById were called multiple times, unnecessarily. This change should not result in any behavior changes.
@cooperka
Copy link
Owner

This looks great @thomasw, thank you. If I have time to make equivalent changes to iOS this week I will, otherwise I'll merge as-is and note that it's Android-only in the readme. Cheers.

@thomasw
Copy link
Contributor Author

thomasw commented Dec 18, 2018

Sweet 👍 As you can see from those test failures, some of the changes which I claimed didn't result in behavior changes actually did result in at least a subtle one. On Android, the implementation behaves the same when given a config containing keys with their values set to undefined vs. a config with undefined keys. I'm assuming the same would be true for the iOS side, but it's definitely something we'll want to verify.

@thomasw
Copy link
Contributor Author

thomasw commented Jan 6, 2019

On Android, the implementation behaves the same when given a config containing keys with their values set to undefined...

This might not actually be true. These changes cause an exception when a snackbar is invoked with an undefined action.

must be evaluating to true when it shouldn't be.

I'll dig a little deeper as soon as I have time to come back to this.

@pozdena
Copy link
Contributor

pozdena commented Jan 29, 2019

Not sure if this is the right way to do this, but I made a PR to merge iOS title color support into thomas' fork. I tested it; works well in my app. I can't speak to the test failure situation, but hopefully you guys can incorporate these changes. Thanks!

thomasw#1

@cooperka cooperka merged commit 5ae0d6e into cooperka:master Jan 31, 2019
@cooperka
Copy link
Owner

Thank you both! I merged this for Android and added @pozdena's iOS change separately in 3ccb463. Both platforms tested and verified.

@thomasw
Copy link
Contributor Author

thomasw commented Jan 31, 2019

Thanks, everyone! 🎆

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.

3 participants