-
Notifications
You must be signed in to change notification settings - Fork 332
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: fallback screen settings button icon rendering with the right color #7764
Conversation
The latest updates on your projects. Learn more about Vercel for Git βοΈ 5 Skipped Deployments
|
@@ -27,6 +26,9 @@ const FallbackCameraBody: React.FC<Props> = ({ | |||
}: Props) => { | |||
const { colors } = useTheme(); | |||
|
|||
// benefit from component scope colors variable for IconSettings | |||
const IconSettings = () => <Icon name="settings" size={16} color={colors.constant.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.
On dark mode, the icon should be black as the background is white, I think that we have some colors for that in the theme, could you find the one we need, please?
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, i will figure it out
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.
@KVNLS in the dark mode (i used the ForceTheme container over the Fallback to simulate it) the button's background is still white , so making the icon white will make it invisible.
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.
you can use colors.neutral.c00 or colors.neutral.c100 :) It will works for both 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.
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.
But you didn't changed const color ?
@@ -27,6 +26,9 @@ const FallbackCameraBody: React.FC<Props> = ({ | |||
}: Props) => { | |||
const { colors } = useTheme(); | |||
|
|||
// benefit from component scope colors variable for IconSettings |
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 understand why you added the comment but I'm not convinced that we need it for this.
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.
so i delete the comment here?
@@ -27,6 +26,9 @@ const FallbackCameraBody: React.FC<Props> = ({ | |||
}: Props) => { | |||
const { colors } = useTheme(); | |||
|
|||
// benefit from component scope colors variable for IconSettings | |||
const IconSettings = () => <Icon name="settings" size={16} color={colors.constant.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.
Could you fix the prettier formatting, please?
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.
yep sure, i'm using the default prettier config in my VSCode.
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.
can you try using neutral.c100 please? :) Instead of using osTheme
if neutral.c100 not working try neutral.c00 I don't remember de right one
it will convert automatically according to the current 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.
sure ! can you please send me (if exists) theming documentation or color palette docs/ref ?
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.
@mcayuelas-ledger done β
718eee9
to
bdbf7ea
Compare
@@ -27,6 +26,9 @@ const FallbackCameraBody: React.FC<Props> = ({ | |||
}: Props) => { | |||
const { colors } = useTheme(); | |||
|
|||
// benefit from component scope colors variable for IconSettings | |||
const IconSettings = () => <Icon name="settings" size={16} color={colors.constant.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.
can you try using neutral.c100 please? :) Instead of using osTheme
if neutral.c100 not working try neutral.c00 I don't remember de right one
it will convert automatically according to the current theme
bdbf7ea
to
e805a7e
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.
lgtm!
β Checklist
npx changeset
was attached.π Description
Fix setting icon rendering color
Before β
After β
β Context
π§ Checklist for the PR Reviewers