Restyled the theme switcher in the nav to a switch rather than a button for improved UX#173
Conversation
sergeychernyshev
left a comment
There was a problem hiding this comment.
Looks like server-side rendering for the switch is not working, it only switches when JS kicks in.
|
It might be a good idea to try to see if it can use radio buttons and apply CSS based on the value of a button rather than a class on the :root element which would allow the switch to work without JS at all. You will still need to set the cookie using JS when it loads though. |
|
Hi @AlexanderChernyshev can you rebase your branch and resolve conflicts? I'd like to review this and get this in if it's ready! If you need to make further changes, that's good too - but once you're ready for a full review let's mark it as such so it's not a draft. Thank you! |
|
Yes, I'll take a look at it and try to resolve it today. Apologies for
dragging it out so long.
…On Tue, Apr 28, 2026 at 10:46 AM Joe Sanchez ***@***.***> wrote:
*joesanchezjr* left a comment (cloudflare/telescope#173)
<#173 (comment)>
Hi @AlexanderChernyshev <https://github.com/AlexanderChernyshev> can you
rebase your branch and resolve conflicts? I'd like to review this and get
this in if it's ready! If you need to make further changes, that's good too
- but once you're ready for a full review let's mark it as such so it's not
a draft. Thank you!
—
Reply to this email directly, view it on GitHub
<#173 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AH33MFPWUI35UXFDCY24HOL4YC757AVCNFSM6AAAAACWGCXZLGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DGMZWGM3TKMJRGM>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
6e504b5 to
875f656
Compare
|
I think we can try merging this now. I and @sergeychernyshev rebased the branch and spent some time resolving issues. It should be ready. I also did a last bit of review of the global CSS. Me and Sergey were worried that Gemini rewrote too much of it there and I was going to double check to see if it did anything terrible. I did find that it re-wrote a bunch of stuff there that affected the app as a whole, not just the results page. Upon checking through it though, it seems these changes should all be fine both technically and from a design standpoint as far as I can tell. I did fix one issue I discovered with Gemini. It mentioned the following:
I had it suggest a fix and rewrite this. So this issue should also be resolved, and the all the changes should be okay now. @sergeychernyshev and @joesanchezjr, feel free to review the changes when you can, and let me know if there are any issues overall or regarding the styling changes. If not, feel free to merge or let me know to merge the PR! |
| <div class="mobile-actions"> | ||
| {!hideThemeSwitcher && ( | ||
| <button class="theme-toggle mobile-theme-toggle" type="button" aria-label="Toggle theme"></button> | ||
| <div class="theme-switch-wrapper"> |
There was a problem hiding this comment.
this seems to be the same html block as on line 37; is there a way to consolidate these to a single definition so we are not repeating code?
Changing the theme switcher element in the top-nav from a button to a switch to improve UX by adjusting the design to be intuitive to the user. This way it is clearer what change will happen when "throwing" the switch without the user needing to interact with it.
Also accounted for mobile view.
Had to make changes to wrangler.jsonc in order to do development on my account.
I moved the account id into only staging and production environments.
Changes were made with the assistance of Google's Gemini AI.