-
Notifications
You must be signed in to change notification settings - Fork 38
Light/Dark Mode #371
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
Light/Dark Mode #371
Conversation
timlenardo
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.
This looks great! Thanks for taking it on Loyd. I left some nits, but I think we can merge this as is.
Two points:
- Why are we prefixing everything with Color (i.e. Color.t_0 instead of just .t_0)?
- There is a follow-up discussion I'd like to have in the next eng sync. I'm not wild about the naming here since it's not immediately obvious to me what "s_0" and "t_3" represent. If I were creating a new component, I wouldn't know which colors to choose. It might be better to have names like 'solid', 'solid100', etc etc, and 'transparent', 'transparent300', etc. It's a bit longer to type, but hopefully we won't be typing for much longer :)
| .styleText(size: 16, weight: .semibold) | ||
| .shadow(color: .white.opacity(0.7), radius: 10, x: 0, y: 0) | ||
| .styleText(size: 16, weight: .semibold, color: Color.white) | ||
| .shadow(color: Color.white.opacity(0.7), radius: 10, x: 0, y: 0) |
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: we may want to use T_N transparent color for the shadow instead of white? I think the white shadow will only show on a dark background!
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.
The white shadow works, because the text is against a permanently blue background!
| .background { | ||
| RoundedRectangle(cornerRadius: Self.width / 2) | ||
| .fill(isHovering ? .gray800 : .black) | ||
| .fill(isHovering ? Color.S_8 : Color.baseBG) |
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: In light mode, I'd like the TetheredButton to match the panel's background color when not hovered and change to a lighter color when hovered.
Currently, when it's not hovered, it's lighter than the panel background:
And then when its hovered, it matches the background:
I think it'd look better if it were the opposite way :)
* Shadow on `SimpleButton`. * Spacing and colors on `WebSearchTab`. * Disabled state in `RemoteModelSection` when verifying provider API token. * `Color` prefixes.
Design
Summary
Onit is finally going light! And it also has a new glassy personality ✨💅
Updates are as follows:
Color.,NSColor., etc.