-
Notifications
You must be signed in to change notification settings - Fork 24.9k
fix(android): displaying dev menu items in light mode #54119
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
if (isEnabled) { | ||
if (UiModeUtils.isDarkMode(context)) android.graphics.Color.WHITE else android.graphics.Color.BLACK | ||
} else { | ||
android.graphics.Color.GRAY | ||
} |
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.
importing for now as that's definitely an improvement, but this should be fixed correctly by using a the color from the theme
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.
Added in 9081797, but not sure if that's what you meant.
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.
I don't think we need the fallback here. It can just be:
if (isEnabled) {
DefaultStyleValuesUtil.getDefaultTextColor(context)
} else {
DefaultStyleValuesUtil.getTextColorSecondary(context)
}
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.
Are we sure that textColor and textColorSecondary is always specified? If not the app crashes when trying to open the dev menu.
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.
Yes it should be part of both Theme.AppCompat
and Theme.Material
so I'd say let's just let the app crash otherwise as there is probably a theme misconfiguration then?
The alternative is to have a private safeGetDefaultTextColor
and safeGetTextColorSecondary
method in this file to handle this fallback
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.
I would go with the second option as it wouldn't be obvious what caused the crash and to be 100% sure that it's gonna work everywhere.
@cortinico has imported this pull request. If you are a Meta employee, you can view this in D84346125. |
78c8be9
to
d9b787e
Compare
Summary: Fixes displaying dev menu items to also take into account the current theme mode. Previously, in light mode, items were not visible because the text was white on a white background for enabled options. ## Changelog: [ANDROID][FIXED] - fixed displaying dev menu items in light mode. Test Plan: Checked in light and dark mode with all options enabled except for "Open DevTools" being disabled. | Before | After | |:------:|:-----:| |<img alt="before-light" src="https://github.com/user-attachments/assets/5a08cb92-8822-4af3-976d-c20db408bcaa" /> |<img alt="after-light" src="https://github.com/user-attachments/assets/02561c15-b6ec-499a-bf83-99bc0c2a9471" />| | Before | After | |:------:|:-----:| | <img alt="before-dark" src="https://github.com/user-attachments/assets/44b2c59e-2fc2-41cc-a599-136ac455afbb" /> |<img alt="after-dark" src="https://github.com/user-attachments/assets/51e6167b-4d3a-4686-a20e-eca17c3b0e10" />| Reviewed By: sbuggay, cortinico Differential Revision: D84346125 Pulled By: coado
Summary: Fixes displaying dev menu items to also take into account the current theme mode. Previously, in light mode, items were not visible because the text was white on a white background for enabled options. ## Changelog: [ANDROID][FIXED] - fixed displaying dev menu items in light mode. Test Plan: Checked in light and dark mode with all options enabled except for "Open DevTools" being disabled. | Before | After | |:------:|:-----:| |<img alt="before-light" src="https://github.com/user-attachments/assets/5a08cb92-8822-4af3-976d-c20db408bcaa" /> |<img alt="after-light" src="https://github.com/user-attachments/assets/02561c15-b6ec-499a-bf83-99bc0c2a9471" />| | Before | After | |:------:|:-----:| | <img alt="before-dark" src="https://github.com/user-attachments/assets/44b2c59e-2fc2-41cc-a599-136ac455afbb" /> |<img alt="after-dark" src="https://github.com/user-attachments/assets/51e6167b-4d3a-4686-a20e-eca17c3b0e10" />| Reviewed By: sbuggay, cortinico Differential Revision: D84346125 Pulled By: coado
d9b787e
to
071f706
Compare
Summary:
Fixes displaying dev menu items to also take into account the current theme mode. Previously, in light mode, items were not visible because the text was white on a white background for enabled options.
Changelog:
[ANDROID][FIXED] - fixed displaying dev menu items in light mode.
Test Plan:
Checked in light and dark mode with all options enabled except for "Open DevTools" being disabled.