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(ui_auth): make light text on _EmailVerificationBadge legible #196

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

qwezey
Copy link

@qwezey qwezey commented Nov 27, 2023

Description

  • 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

Related Issues

This PR resolves issue #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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (melos run analyze) does not report any problems on my PR.
  • All unit tests pass (melos run test:unit:all doesn't fail).
  • I read and followed the Flutter Style Guide.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

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.
@EArminjon
Copy link

EArminjon commented Nov 28, 2023

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.
It could be really fantastic to be able to have the hand on every things :/ !

@qwezey
Copy link
Author

qwezey commented Nov 28, 2023

@EArminjon For this PR, do you think I should allow the client to override the whole decoration property of the Container or only the color property of the BoxDecoration?

@EArminjon
Copy link

I'm not a maintainer, it's just a global question around this package and maybe the real fix behind this needs.

@lesnitsky
Copy link
Member

lesnitsky commented Nov 29, 2023

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.

Styling API is in place and contributions on making anything configurable are welcome.

In general, here's how styling should be taken care of:

  • extend FirebaseUIStyle
  • override applyToMaterialTheme or applyToCupertinoTheme if necessary (see this example)
  • define a property on a relevant widget (like here)
  • define Set<FirebaseUIStyle> styles on a screen level (like here)
  • use FirebaseUIStyle.ofType<T extends FirebaseUIStyle> (like here) to get a relevant style object from build context

In this specific issue, we could create an EmailVerificationBadgeStyle object and have a configurable backgroundColor defined there.

@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),
Copy link
Member

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.

@russellwheatley
Copy link
Member

Hey @qwezey - will you be following up on this PR?

@russellwheatley russellwheatley added the blocked: customer response Waiting for customer response, e.g. more information was requested. label Apr 12, 2024
@cmjordan42
Copy link

cmjordan42 commented Apr 24, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked: customer response Waiting for customer response, e.g. more information was requested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants