-
Notifications
You must be signed in to change notification settings - Fork 56
Improve consistency when handling colors in surveys. #233
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
base: main
Are you sure you want to change the base?
Conversation
Instructions and example for changelogPlease add an entry to ## Next
- Improve consistency when handling colors in surveys ([#233](https://github.com/PostHog/posthog-flutter/pull/233))If none of the above apply, you can opt out of this check by adding |
|
@PostHog/team-surveys hey guys, since you recently worked on themes for react-native, mind taking a look at this as well? |
|
thank you for this PR!!! taking a look now 😄 |
|
Thank you @adboio ! That's an even better solution than before! |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
c739d2f to
1f5f2cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of comments, lgtm. Did not do a test run though
Also we should run a make format before merging
| : Colors.black.withAlpha(128), | ||
| ? appearance.choiceButtonTextColor | ||
| : appearance.choiceButtonTextColor | ||
| .withAlpha(128), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Right above we use withValues(alpha: 0.5), which I think produces the same but worth standardizing and being consistent here
|
|
||
| // Input background: use override, or slight adjustment for white backgrounds | ||
| final inputBackgroundColor = _colorFromHex(appearance?.inputBackground) ?? | ||
| (backgroundColor == Colors.white |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will only match only purely white background. Should we be testing for high luminance values instead? e.g if backgroundColor.computeLuminance() > 0.95
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, this matches all the other SDKs, but computeLuminance looks like a much better approach - can try to do something similar in other sdks too!
| if let submitButtonTextColor = appearance.submitButtonTextColor { | ||
| appearanceDict["submitButtonTextColor"] = submitButtonTextColor | ||
| } | ||
| if let textColor = appearance.textColor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did not check but is Android side consistent with these changes as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
react-native, ios, and (now) flutter will all be consistent once this is merged
i didn't think android had any built-in survey UI rendering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, just the infra that powers the other SDKs
1f5f2cf to
ae18fcb
Compare
marandaneto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just unblocking here since @ioannisj did the review already
Co-authored-by: Tate <wintermydog@gmail.com>
ab56ace to
5a48b48
Compare
5a48b48 to
e8a0962
Compare





Co-authored-by: Tate wintermydog@gmail.com
💡 Motivation and Context
This was written by Tate, but due to our projects not having the ability to allow external contributors at the moment I am sort of sponsoring this. I'll let Tate explain:
💚 How did you test it?
I, Kyle, modified the style of the example app on iOS and tested against a sample survey of varying colors and styles.
📝 Checklist