-
Notifications
You must be signed in to change notification settings - Fork 30
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(components-native): AtlantisThemeContext for mobile #2387
Conversation
Deploying atlantis with
|
Latest commit: |
7e177f9
|
Status: | ✅ Deploy successful! |
Preview URL: | https://1e53e2fb.atlantis.pages.dev |
Branch Preview URL: | https://job-116234-theme-context-for.atlantis.pages.dev |
useAtlantisTheme, | ||
} from "@jobber/components-native"; | ||
|
||
export default { |
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.
Added some basic stories. They won't look correct in dark mode until we update the entire library to use buildThemedStyles
.
Screen.Recording.2025-02-20.at.1.46.26.PM.mov
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.
Note, after I recorded this video I reverted the Button style changes. @Aiden-Brine's PR will handle that!
children, | ||
dangerouslyOverrideTheme, | ||
}: AtlantisThemeContextProviderProps) { | ||
// TODO: check last saved theme from local/device storage |
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 a TODO for the future when we actually do dark mode in JM
Deploying atlantis with
|
Latest commit: |
9fe8b36
|
Status: | ✅ Deploy successful! |
Preview URL: | https://325d3cc1.atlantis.pages.dev |
Branch Preview URL: | https://job-116234-theme-context-for.atlantis.pages.dev |
@@ -18,7 +18,7 @@ const skeletonHTML = (theme: Theme, type: "web" | "mobile") => { | |||
return ` | |||
|
|||
<!DOCTYPE html> | |||
<html data-theme="${type === "web" ? theme : "light"}" data-type="${type}"> | |||
<html data-theme="${theme}" data-type="${type}"> |
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 got dark mode working with mobile iframes:
Screen.Recording.2025-02-21.at.9.42.13.AM.mov
if (iframeMobile.current) { | ||
const iframeMobileWindow = iframeMobile.current.contentWindow; | ||
iframeMobileWindow?.postMessage({ type: "updateTheme", theme }, "*"); | ||
} |
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.
When the docs site theme is toggled, this also toggles the mobile iframe preview just like the web one above.
Deploying atlantis with
|
Latest commit: |
3c05faf
|
Status: | ✅ Deploy successful! |
Preview URL: | https://4fdb6165.atlantis.pages.dev |
Branch Preview URL: | https://job-116234-theme-context-for.atlantis.pages.dev |
Deploying atlantis with
|
Latest commit: |
a7d4432
|
Status: | ✅ Deploy successful! |
Preview URL: | https://395668db.atlantis.pages.dev |
Branch Preview URL: | https://job-116234-theme-context-for.atlantis.pages.dev |
Deploying atlantis with
|
Latest commit: |
5348136
|
Status: | ✅ Deploy successful! |
Preview URL: | https://89b8698d.atlantis.pages.dev |
Branch Preview URL: | https://job-116234-theme-context-for.atlantis.pages.dev |
Deploying atlantis with
|
Latest commit: |
556789c
|
Status: | ✅ Deploy successful! |
Preview URL: | https://a3b0d0e6.atlantis.pages.dev |
Branch Preview URL: | https://job-116234-theme-context-for.atlantis.pages.dev |
This reverts commit 556789c.
* Force the theme for this provider to always be the same as the provided theme. Useful for sections that should remain the same theme regardless of the rest of the application's theme. | ||
* This is dangerous because the children in this provider will not be able to change the theme. | ||
*/ | ||
readonly dangerouslyOverrideTheme?: Theme; |
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.
Probably too late for this component because I know we use this language on the web theme provider but ideally should this be "unsafe" instead of "dangerous" to be more consistent with our current naming or is there a difference between an unsafe action and a dangerous action? This definitely isn't for this PR but I wanted to raise it to chat about
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.
Checking the code I think this is the only place we use "dangerous"
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, good point. This predates our decisions around using unsafe terminology. I believe the original intent for using dangerously is that it's similar to other react props, like dangerouslySetInnerHTML
.
I'll create a ticket to rename these props in atlantis and consumers.
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.
Made the ticket, it's in our backlog 👍
@@ -61,6 +61,50 @@ function ThemedComponent() { | |||
} | |||
``` | |||
|
|||
### Usage for mobile |
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.
The usage sections give enough information to figure out how to use both web and mobile but how about a small section to highlight the differences so that if someone who is familiar with one is looking at the other they can quickly see the differences in usages?
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.
Good point, will add this today!
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.
Add some docs above!
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 left two non-blocking questions but otherwise this looks really nice!
|
||
On web, you'll need to import the `updateTheme` function and call it with the | ||
new theme. This is a separate function because it synchronizes the theme update | ||
across all providers in various react trees (due to our island architecture). |
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.
Should this say "to handle cases where a React Island architecture is used" since there is potential that someone outside of Jobber is reading these docs?
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 outlining specifics about our architecture might be fine. We do reference internal specifics in other places, see https://atlantis.getjobber.com/content/voice-and-tone for example.

While I don't think this change is necessary, I'll make the adjustment as it's pretty small and still gets the point across.
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.
Updated!
Motivations
This PR adds a new mobile component:
AtlantisThemeContextProvider
. This component behaves similar to our web version with the exact same API.We also expose a
useAtlantisTheme()
hook. This is slightly different from the web version because it also returns asetTheme()
method which allows you to change the theme globally. The web version is a little more complex because it needs to dispatch a global event (viaupdateTheme()
) that all theme providers (in various distinct react roots) listen for. Mobile doesn't need that complexity as we only have one react tree and not a bunch of disparate islands.Changes
Added
Testing
There's not much testing to do. This is a new component that will not affect JM at all until we start using it, which will be in followup PRs.
Changes can be tested via Pre-release