-
Notifications
You must be signed in to change notification settings - Fork 101
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(ui_auth): make light text on _EmailVerificationBadge legible #196
fix(ui_auth): make light text on _EmailVerificationBadge legible #196
Conversation
Reduced the opacity of the badge's background to make light text in a dark theme more legible. Dark text in a light theme is still legible. The reduced opacity also makes the color more appealing.
I think it could be better to expose this color through a dedicated parameter. In fact, it could be good to have the possibility to override every UI widget of this package to custom them as close as client app current design. |
@EArminjon For this PR, do you think I should allow the client to override the whole |
I'm not a maintainer, it's just a global question around this package and maybe the real fix behind this needs. |
Styling API is in place and contributions on making anything configurable are welcome. In general, here's how styling should be taken care of:
In this specific issue, we could create an @qwezey would you be willing to update your PR accordingly? |
@@ -387,7 +387,7 @@ class _EmailVerificationBadgeState extends State<_EmailVerificationBadge> { | |||
children: [ | |||
Container( | |||
decoration: BoxDecoration( | |||
color: Colors.yellow, | |||
color: Colors.yellow.withOpacity(0.5), |
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.
please make this configurable as mention here.
Hey @qwezey - will you be following up on this PR? |
@lesnitsky @russellwheatley Can we proceed with merging this PR? It is a bug fix but requests to incorporate non-critical enhancements seem to be stalling it. We are at a 0 currently and this change seems like it would get us to a 1, so it seems prudent to keep any enhancements to get to a 2 separate from this. |
@cmjordan42 - that's a fair point, I'd rather have an improvement at this moment in time rather than wait for an incoming PR with a more robust solution. |
Many thanks, @russellwheatley |
Description
Related Issues
fixes #18
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
).This will ensure a smooth and quick review process. Updating the
pubspec.yaml
and changelogs is not required.///
).melos run analyze
) does not report any problems on my PR.melos run test:unit:all
doesn't fail).Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?