-
Notifications
You must be signed in to change notification settings - Fork 35
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
Configure darkmode with props #28
Configure darkmode with props #28
Conversation
🦋 Changeset detectedLatest commit: dd68c15 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
8c0f39c
to
7a27f49
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.
Great work @bpingris, I really appreciate the contribution ❤️
Left some feedbacks to increase the flexibility of the code.
We'll also need to:
- Forward the prop in
Calendar.List
- Add a
Calendar
story to showcase/test the functionality.
Let me know if you have any FUP questions!
export const useCalendarContext = () => { | ||
const context = useContext(calendarContext); | ||
|
||
if (!context) { | ||
throw new Error( | ||
"useCalendarContext must be called inside <calendarContext.Provider>" | ||
); | ||
} | ||
return 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.
can we move this to a useCalendarTheme
file?
@@ -68,8 +68,25 @@ export interface CalendarProps extends UseCalendarParams { | |||
calendarMonthHeaderHeight?: number; | |||
/** Theme to customize the calendar component. */ | |||
theme?: CalendarTheme; | |||
/** Enable dark mode */ | |||
darkMode?: 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.
Let's make this more flexible by changing to:
/**
* When set, Flash Calendar will use this color scheme instead of the system's
* value (`light|dark`). This is useful if your app doesn't support dark-mode,
* for example.
*
* We don't advise using this prop - ideally, your app should reflect the
* user's preferences.
*/
colorSchemeToOverride?: ColorSchemeName;
const calendarContext = createContext<{ darkMode?: boolean } | undefined>( | ||
undefined | ||
); |
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.
can we move this to a useCalendarTheme.ts
file?
} | ||
|
||
const calendarContext = createContext<{ darkMode?: boolean } | undefined>( |
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.
instead of darkMode
, let's use the idea from above:
type CalendarContextType = {
/** The overridden color scheme */
colorScheme?: ColorSchemeName
}
const calendarThemeContext = createContext<CalendarContextType | undefined>(undefined)
|
||
if (!context) { | ||
throw new Error( | ||
"useCalendarContext must be called inside <calendarContext.Provider>" |
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.
nit: let's also change the error message to reflect the new context name
@@ -168,7 +190,11 @@ export const Calendar = memo( | |||
*/ | |||
}, [calendarActiveDateRanges, calendarMonthId]); | |||
|
|||
return <BaseCalendar {...props} calendarMonthId={calendarMonthId} />; | |||
return ( | |||
<calendarContext.Provider value={{ darkMode }}> |
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 need to memo this value to avoid re-renders:
const calendarThemeContextValue = useMemo<CalendarThemeContext>(() => ({ colorScheme: colorSchemeToOverride }), [colorSchemeToOverride])
@MarceloPrado I've made the changes but I think I'm stuck with the storybook |
@bpingris I ran your PR locally and saw this error: One thing I realized: we don't want to throw an error if someone forgets to add the context. One of Flash Calendar's principles is that's easy to customize/compose. Adding a context consumers have to wrap is a step against that direction. What do you think of this approach? import { createContext, useContext } from "react";
import type { ColorSchemeName } from "react-native";
export interface CalendarThemeContext {
/** The overridden color scheme */
colorScheme?: ColorSchemeName;
}
export const calendarThemeContext = createContext<CalendarThemeContext>({
colorScheme: undefined,
});
export const useCalendarThemeContext = () => {
const context = useContext(calendarThemeContext);
return context;
}; I ran your code using the context above and everything seems to work as expected (the calendar stops listening to the theme): CleanShot.2024-04-12.at.15.03.05.mp4 |
hey sorry for the delay! |
e30d38e
to
dd68c15
Compare
Hi @bpingris!
let me know if you have further questions! |
hey!
this is a follow up pr after this tweet
I though about such api where we would only need to pass a
darkMode: bool
prop, if none is given we still use theuseColorScheme
hook call etc.what do you think?
I haven't had the time to test it yet though! so I leave it as a draft for now, thanks!