-
Notifications
You must be signed in to change notification settings - Fork 212
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
Add the UI toggle for changing theme #4840
Conversation
This PR is drafted till #4810 is merged and labelled as "⛔️ status: blocked" (to prevent accidentally merging it). Once that is merged, it will be rebased and undrafted. Feel free to review in the meantime, I do not anticipate UI/functionality changes. |
2d5fea3
to
a3f775f
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.
Lovely! There are a few small issues I'm noticing:
- The "system" mode icon doesn't reflect the user's preference on SSR, it is always light.
- There's an icon sizing/spacing issue:
- For the first issue, perhaps making the component clientonly would fix it
I really like how you're displaying the parenthetical of what the actual system value is, nice touch!
90c1b9a
to
a4efc3a
Compare
@zackkrida that is so weird, I'm not seeing the select field sizing issue on any major browser! This is Safari, Arc (Chromium) and Firefox: Safari has some wildly different problems going on, though. |
@dhruvkb it looks like removing I'm seeing it in Chrome and Firefox on Fedora. In any case, I think we can safely remove the overflow hiding. |
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.
Nice to be able to change the theme from the UI!
I have 2 blocking requests:
-
The
VThemeSelect
should not detect the user's color scheme. It should only use the value from the UI store, and update it.
app.vue
(anderror.vue
) as the entrypoints to the application would be the best place for the detection of the user's color scheme. We update the UI store layout properties (breakpoint and width) there. -
The
select
inside theVSelectField
component does not set theoutline
style or width onfocus-visible
, but we do set its color. So with the default values, the focus ring has a white outline. Adding afocus-visible:outline-none
to theselect
element could fix this white ring around the pink one:
|
a0f7587
to
6dc18b5
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.
The functionality is perfect ✨ It's nice that the composable encapsulates handling of the color mode and we don't have to update the pages.
In dark mode, I can see a ring around the select field focus border in Arc, Chrome and Safari. Firefox is the only browser that displays the focus border correctly. I'm not blocking on this because this is a problem that existed with the language select, and should have a separate issue.
Fixes
Fixes #4309 by @zackkrida
Fixes #4155 by @fcoveram
Description
This PR
VSelectField
component to hide the active choiceTesting Instructions
Checklist
Update index.md
).main
) or a parent feature branch.ov just catalog/generate-docs
for catalogPRs) or the media properties generator (
ov just catalog/generate-docs media-props
for the catalog or
ov just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin