Skip to content
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

Feature/dark mode swithc #8

Merged
merged 10 commits into from
Dec 17, 2023
Merged

Feature/dark mode swithc #8

merged 10 commits into from
Dec 17, 2023

Conversation

Pdzly
Copy link
Member

@Pdzly Pdzly commented Dec 15, 2023

No description provided.

@Pdzly Pdzly self-assigned this Dec 15, 2023
@Pdzly Pdzly requested a review from kgilles December 15, 2023 20:42
@Pdzly Pdzly marked this pull request as ready for review December 15, 2023 20:42
import { useLocalStorage } from './localstorage';

export function useTheme() {
return useLocalStorage('theme', typeof window !== 'undefined' && window.matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing this with dark mode, I notice a delay as we always show light mode on initial page load and then half a second later it changes to dark mode. Almost like a flashbang effect.

While we should definitely keep this switch, we need to find a way to allow tailwind to listen for the browser mode setting again. Something broke when adding MT but it makes no sense for them to block tailwind's default dark mode rules. Had to be some way around it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's also preventing certain tailwind response threshold classes from working, it causes the bottom nav bar to overflow in narrow screens.
image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weirdly i dont get it. Or what do you mean it directly?

src/components/theme-switch/index.tsx Show resolved Hide resolved
src/components/theme-switch/index.tsx Outdated Show resolved Hide resolved
];

return (
<div className="flex justify-center items-center">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make this a button with an aria-label to make it more accessible.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not do that to the whole menu i would do a aria label to the button and the menu

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good thought. Putting it under MenuHandler like you did makes a lot more sense.

However, since we want it to be an invisible button it'd be easier to use the default button element rather than one from MT that requires us to style it to make it invisible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The menu dropdown stays open after page navigation. I'd prefer it closes when a user clicks on an option that takes them to another page. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be annoying for the user if he changes the darkmode. And i rather like that it stays open, if you didnt wanted to go there you can click to go somewhere else.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right we don't close it when they change modes. I was thinking about when they navigate to their profile page from the menu dropdown for example. If they use the profile menu to navigate somewhere else then they're done using it.

It's also staying open if I click to show it but then click another link on the page, like "Create post".

];

return (
<div className="flex justify-center items-center">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good thought. Putting it under MenuHandler like you did makes a lot more sense.

However, since we want it to be an invisible button it'd be easier to use the default button element rather than one from MT that requires us to style it to make it invisible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right we don't close it when they change modes. I was thinking about when they navigate to their profile page from the menu dropdown for example. If they use the profile menu to navigate somewhere else then they're done using it.

It's also staying open if I click to show it but then click another link on the page, like "Create post".

@Pdzly Pdzly enabled auto-merge (squash) December 17, 2023 19:33
@kgilles kgilles self-requested a review December 17, 2023 19:36
@Pdzly Pdzly merged commit cdcd654 into main Dec 17, 2023
1 check passed
@Pdzly Pdzly deleted the feature/dark-mode-swithc branch December 17, 2023 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants