-
-
Notifications
You must be signed in to change notification settings - Fork 430
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
feat: flowbite theme context provider #61
Conversation
src/components/Flowbite/index.tsx
Outdated
export type FlowbiteProps = PropsWithChildren<{ | ||
children: any; | ||
dark?: boolean; | ||
usePreferences?: boolean; |
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 think we should group the above two props because they are both related to the theme context.
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.
What would the benefits be?
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.
Later on, if we add more props to this component it might become unclear what every prop is doing.
src/contexts/ThemeContext.tsx
Outdated
|
||
interface ThemeContextInterface { | ||
mode?: Mode; | ||
toggleMode?: any; |
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.
Let's not use the type any
anywhere and have always the exact type we need, for example here we need () => void
src/hooks/useDarkMode.ts
Outdated
if (!mode) return; | ||
const newMode = mode == 'light' ? 'dark' : 'light'; | ||
document.documentElement.className = ''; | ||
document.documentElement.classList.add(newMode); |
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.
We can replace the above three lines by document.documentElement.classList.toggle('dark')
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.
hoho, didn't know about this one.
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.
But it will change a lot of the behaviour. 🤔
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.
Yeah, I fact I could change it, but then it will not work as I'm expecting. I'll remove 3 lines here, and add a few more in other places. Don't see it as a 🏆.
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 don't see changing these lines as an optimization, I see a bug here that needs to be fixed, because if you do this affectation document.documentElement.className = '';
you might be breaking some behavior in the product.
src/contexts/ThemeContext.tsx
Outdated
children: React.ReactNode; | ||
value?: any; | ||
} | ||
type ThemeProviderProps = ProviderProps<{}>; |
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.
Still we should replace {}
with ThemeContextProps
NOTE: I'm a little bit busy this week, but once I have some free time I'll continue work on this. |
@rluders I am noticing a pattern with I have no idea if this is the best/ideal way to do this: const flowbiteTheme = {
components: {
Sidebar: "fixed overflow-auto top-16 h-screen lg:sticky lg:block"
}
}
...
<Flowbite theme={flowbiteTheme}>
...
<Sidebar> // className inherits flowbiteTHeme.components.Sidebar
..
</Sidebar>
</Flowbite> |
So, this is basically what I'm doing in this PR. Give the possibility to overwrite some component styles by creating themes. Not sure yet if we want to also put some filters to avoid some design system changes by the theme, but... I'm thinking about it, lets see how it all will work. |
There are still pieces to validate on this experiment, some results from code review, others from improvements and features that I want to include into it. So it is probably far to be completed. However, feel free to leave any early feedback. |
@tulup-conner please, give it a try |
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 is so exciting 💯 I actually haven't added Accordions to my site yet LOL but I will now. In the meantime I am excited to test the rest of the components.
This PR adds some theme context provider to handle splitting the part of the code that handles the dark mode and to be able to support some other theme features in the future.
DISCLAIMER: THIS IS AN EXPERIMENT, WE ARE NOT SURE THAT IT BECOME PART OF FLOWBITE REACT YET.