-
Notifications
You must be signed in to change notification settings - Fork 95
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
Dark mode feature added to mirrorful editor #380
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@Bismay5467 is attempting to deploy a commit to the North Park Labs Team on Vercel. A member of the Team first needs to authorize it. |
<img src="/simple_logo.png" style={{ width: '150px' }} /> | ||
</motion.div> | ||
) : colorMode === 'dark' ? ( | ||
<img src="/logo-dark-mode.png" style={{ width: '150px' }} /> |
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.
@alexdanilowicz I am facing some issue trying to toggle the mirrorful logos between light and dark mode settings. Please, could you assist me in resolving this issue?
@alexdanilowicz do let me know your thoughts on this design ! |
@alexdanilowicz There are significant differences in the UIs for the dark and bright modes. |
@@ -36,7 +44,7 @@ export function AlertDialogDelete({ | |||
onClose={onClose} | |||
> | |||
<AlertDialogOverlay> | |||
<AlertDialogContent> | |||
<AlertDialogContent bg={backgroundColor}> |
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'm not sure if this is the right way to do it. This way we'll need to add a prop to every element we support and then drill this value to the element. Can we define the themes and the current theme in a top level context or zustand file somewhere and have that imported in the actual elements. This way we don't need to keep on passing the color. The actual component can refer this zustand store to identify what the currently chosen theme is and style it accordingly. @alexdanilowicz Thoughts?
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.
@Pranav2612000 Yes, it makes perfect sense to me.
Although I haven't used zustand before, I'm quite interested in learning about it.
@alexdanilowicz If required I can push a PR with the necessary changes.
Hi, great first steps. Ideally, we use Mirrorful itself to power our dark mode themeing. @Pranav2612000 is correct in that it seems weird to pass props and instead should be top-level themeing, but even the top-level themeing is not perfect Let me talk to @Teddarific as we will be adding a semantic tab soon to Mirrorful and then can use Mirrorful in Mirrorful. As-is, this PR is a GREAT START (thanks @Bismay5467), but I don't think using zustand, nor manually defining the theme in each file is the perfect solution, so let's hold on off this for a bit. I'm going to update the issue. |
Going to close this for the time being, in alignment with what was written above. |
Resolves #61