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

Switch between light and dark themes in Prebuilt UI #231

Open
ahkhanjani opened this issue Nov 24, 2023 · 17 comments
Open

Switch between light and dark themes in Prebuilt UI #231

ahkhanjani opened this issue Nov 24, 2023 · 17 comments
Labels
bug Something isn't working

Comments

@ahkhanjani
Copy link

I'm currently using .setTheme() method to change the colors object entirely on theme change. But since Daily already provides a means to initialize the objects themselves, using that sounds more reasonable. Except I couldn't find any documented way of switching between light and dark modes.

The reason I'm interested in this approach is that, when the iframe is initializing, if the website is in dark mode, there will be a white background flash for a moment. I can't figure out which element exactly is causing this. Event setting the initial colors to the dark mode colors doesn't do the trick. So maybe Daily needs both themes initially + a way to know that we're in dark mode so that it can use that color scheme before our useEffect kicks in and sets the colors object.

Here's my code:

const LIGHT_THEME: DailyThemeColors = {
  background: "#FFFFFF",
  backgroundAccent: "#FFFFFF",
  mainAreaBg: "#FFFFFF",
  mainAreaText: "#000000",
  mainAreaBgAccent: "#FFFFFF",
  accent: "#2566FF",
  border: "#E2E8F0",
  baseText: "#000000",
  accentText: "#FFFFFF",
  supportiveText: "#000000",
};

const DARK_THEME: DailyThemeColors = {
  background: "#020817",
  backgroundAccent: "#020817",
  mainAreaBg: "#020817",
  mainAreaText: "#FFFFFF",
  mainAreaBgAccent: "#020817",
  accent: "#2566FF",
  border: "#1E293B",
  baseText: "#F8FAFC",
  accentText: "#FFFFFF",
  supportiveText: "#FFFFFF",
};

const CALL_OPTIONS: DailyFactoryOptions = {
  strictMode: true,
  showLeaveButton: true,
  showFullscreenButton: true,
  iframeStyle: {
    top: "0",
    left: "0",
    width: "100%",
    height: "100%",
    border: "0",
    borderRadius: "0",
  },
};

function getCurrentTheme(resolvedTheme: string | undefined) {
  return { colors: resolvedTheme === "dark" ? DARK_THEME : LIGHT_THEME };
}

export default function Call() {
  const { resolvedTheme } = useTheme();

  const [callFrame, setCallFrame] = useState<DailyCall | null>(null);
  const callRef = useRef<HTMLDivElement>(null);

  useEffect(() => {
    if (!callFrame) return;
    void callFrame.setTheme(getCurrentTheme(resolvedTheme));
  }, [callFrame, resolvedTheme]);

  useEffect(() => {
   	if (!callRef.current) return;
    const newCallFrame = DailyIframe.createFrame(callRef.current, CALL_OPTIONS);
	// ...
    void newCallFrame.join({ ... });
  }, [createAndJoinCall]);

  return <div ref={callRef} />;
}
@ahkhanjani ahkhanjani added the bug Something isn't working label Nov 24, 2023
@Regaddi
Copy link
Contributor

Regaddi commented Nov 29, 2023

Hey @ahkhanjani,

you'll want to pass both objects to createFrame. Prebuilt internally relies on the @media (prefers-color-scheme: dark) media query to determine if light or dark colors should be applied.

To achieve that, pass both theme objects like this:

const CALL_OPTIONS: DailyFactoryOptions = {
  strictMode: true,
  showLeaveButton: true,
  showFullscreenButton: true,
  iframeStyle: {
    top: "0",
    left: "0",
    width: "100%",
    height: "100%",
    border: "0",
    borderRadius: "0",
  },
  theme: {
    light: {
      colors: LIGHT_THEME,
    },
    dark: {
      colors: DARK_THEME,
    }
  }
};

You don't have to call setTheme after that, unless you want to change the color palette.

Let me know if this solves the issue for you.

Best,
Christian

@ahkhanjani
Copy link
Author

Hi @Regaddi. Thank you for the explanation.

Prebuilt internally relies on the @media (prefers-color-scheme: dark) media query to determine if light or dark colors should be applied.

Is this part mentioned in the docs? I don't remember seeing this. If it's not mentioned I think we should add it there. This might be natural for vanilla CSS users but for those of us who use things like Tailwind and data attribute tricks it's not so obvious.

Best regards

@Regaddi
Copy link
Contributor

Regaddi commented Nov 29, 2023

While the theme property is listed as a property for the Daily Call Client, I'm noticing that the link to setTheme() seems broken. I'll make sure to fix this in our docs. While I'm there, let me add a small highlight that the color scheme is inferred from the user setting.

@ahkhanjani ahkhanjani reopened this Dec 1, 2023
@ahkhanjani
Copy link
Author

ahkhanjani commented Dec 1, 2023

Hey @Regaddi,

you'll want to pass both objects to createFrame. Prebuilt internally relies on the @media (prefers-color-scheme: dark) media query to determine if light or dark colors should be applied.

It turns out that this doesn't work when we want to switch between light/dark modes independent of user's system settings, since we can't change prefers-color-scheme using JavaScript.

I think we should have an option like:

{ darkMode?: boolean | undefined; setDarkMode: (darkMode: boolean | undefined) => void }

So that when it's undefined, it defaults to prefers-color-scheme, but when specified, it chooses the specified theme.

The reason I'm interested in this approach is that, when the iframe is initializing, if the website is in dark mode, there will be a white background flash for a moment.

If Daily does what I think it does, it should solve this issue. But even if they are unrelated, this should improve the DX of working with light/dark themed apps.

Best regards

@Regaddi
Copy link
Contributor

Regaddi commented Dec 1, 2023

Hey @ahkhanjani,

in this case you'll want to initialize the callFrame with the appropriate color theme, as set in your custom theme settings:

  useEffect(() => {
   	if (!callRef.current) return;
    const newCallFrame = DailyIframe.createFrame(callRef.current, {
      ...CALL_OPTIONS,
      theme: getCurrentTheme(resolvedTheme),
    });
	// ...
    void newCallFrame.join({ ... });
  }, [createAndJoinCall, resolvedTheme]);

That will pretty much ignore any prefers-color-scheme media queries in Prebuilt and will initialize the frame with the correct color information.

Let me know how that goes!

@ahkhanjani
Copy link
Author

in this case you'll want to initialize the callFrame with the appropriate color theme, as set in your custom theme settings:

  useEffect(() => {
   	if (!callRef.current) return;
    const newCallFrame = DailyIframe.createFrame(callRef.current, {
      ...CALL_OPTIONS,
      theme: getCurrentTheme(resolvedTheme),
    });
	// ...
    void newCallFrame.join({ ... });
  }, [createAndJoinCall, resolvedTheme]);

@Regaddi That's a solution. The reason I didn't go for this approach was the problems discussed in #229. Since resolvedTheme can change, the Daily instance will need to be cleaned up on theme change. That's why I avoided including anything non-related to Daily's initialization effect function.

But with that being said, I think there is not much to do until #229 is solved and we can handle everything including theme change via the new internal hook or something.

Thanks for your patience

@Regaddi
Copy link
Contributor

Regaddi commented Feb 6, 2024

Hey @ahkhanjani,

following up here to check if there's anything else you need to have this issue resolved, since #229 is resolved now.

Best,
Christian

@ahkhanjani
Copy link
Author

Hey @Regaddi,

following up here to check if there's anything else you need to have this issue resolved, since #229 is resolved now.

I'm having some trouble setting the new hook up. Like mentioned here. daily-react v0.17.1 has fixed a good amount of them but I still can't get it to work.

After successfully migrating and testing this issue I will let you know.
Best regards

@ahkhanjani
Copy link
Author

ahkhanjani commented Feb 6, 2024

Now that it's working, I tried passing the theme directly to the new hook options:

const { resolvedTheme } = useTheme();

const callFrame = useCallFrame({
  ...,
  options: { ...CALL_OPTIONS, theme: getCurrentTheme(resolvedTheme) },
});

The flash is still there. I can't tell what exactly causes it but I'm guessing it's one of these two scenarios:

  1. It takes one render cycle for the theme to be resolved (from local storage, etc.).
  2. The base of Daily's iframe is white by default and does not get modified by the developer options. So until the actual call loads, you will see a white screen.

Also passing the theme -which can change at any time by the user- to the call options seems to trigger a full reload of the call frame. So I'm assuming nothing passed to useCallFrame should ever change.

If the problem is number 2, the fix should be as straightforward as changing the background color of the initial parent of the call to transparent. Please let me know if that's not the case.

Thanks for your hard work,
Amir

@Regaddi
Copy link
Contributor

Regaddi commented Feb 7, 2024

@ahkhanjani I'm working on a fix to prevent the flash of un-themed colors you are experiencing. When the iframe is created initially, it has visibility: hidden applied and that style should only be removed when the initial configuration has been exchanged with Daily Prebuilt.

Also passing the theme -which can change at any time by the user- to the call options seems to trigger a full reload of the call frame. So I'm assuming nothing passed to useCallFrame should ever change.

You'll only want to pass the initial theme to useCallFrame() and configure shouldCreateInstance() to not create new instances after the initial config has been applied. A boolean React state could be enough.
Here's some untested sample code to transport the idea:

const [initialConfigApplied, setInitialConfigApplied] = useState(false);
const callFrame = useCallFrame({
  parentEl: callRef.current,
  options: { ...CALL_OPTIONS, theme: getCurrentTheme(resolvedTheme) },
  shouldCreateInstance: useCallback(() => !initialConfigApplied, [initialConfigApplied]),
});
useEffect(() => {
  if (!callFrame) return;
  callFrame.once('loaded', () => setInitialConfigApplied(true));
}, [callFrame]);

To update the theme at runtime you'll want to use setTheme(). This avoids your call to reload unintentionally and only updates the theme.

@ahkhanjani
Copy link
Author

Should initialConfigApplied be used with the ready state we used here? Like ready && !initialConfigApplied

You'll only want to pass the initial theme to useCallFrame() and configure shouldCreateInstance() to not create new instances after the initial config has been applied.

I just tested this and it does solve the reloading problem. But visually nothing has changed. I assume this will be needed after the fix?
I've been listening to the theme change like this:

useEffect(() => {
  if (!callFrame) {
    return;
  }
  
  void callFrame.setTheme(getCurrentTheme(resolvedTheme));
}, [callFrame, resolvedTheme]);

React makes it so difficult to actually understand what happens when. So I wonder if initialConfigApplied state will make any difference here.

@Regaddi
Copy link
Contributor

Regaddi commented Feb 19, 2024

Hey @ahkhanjani,

I think all the issues you ran into should be solved by now.
Can you try once more and report back if there are any outstanding issues?

Should initialConfigApplied be used with the ready state we used here? Like ready && !initialConfigApplied

This shouldn't be required anymore, unless the callRef is mounted after the call frame is being setup.

@ahkhanjani
Copy link
Author

Hi @Regaddi. The rough edges and bugs all seem to be fixed. From this issue, the white flashing problem still remains.

@Regaddi
Copy link
Contributor

Regaddi commented Feb 20, 2024

Hey @ahkhanjani,

could you share a reproduction of the flashing issue? It should be resolved already and I can't reproduce it myself.
Ideally you could share a codesandbox or codepen that demonstrates the issue.

Thanks in advance!

@ahkhanjani
Copy link
Author

My apologies @Regaddi. I made a codesandbox repro and as you said didn't see any of the two problems I'm facing. Turns out the theme library I'm using, next-themes, specifically the ThemeProvider context provider causes the flashing effect. Although this made a good discussion, I apologize for the unnecessary ones this has caused.
I still haven't figured out the other issue so I will follow it there.

Thank you.

@ahkhanjani ahkhanjani reopened this Mar 2, 2024
@ahkhanjani
Copy link
Author

ahkhanjani commented Mar 2, 2024

Hey @Regaddi, sorry to bother again.
I honestly don't know where to put this because a lot of issues come up at once every time I give this another try. I've tried everything including changing the whole theme library that uses another method, and both vanilla daily-js and daily-react. The flicker is still there for some reason and now the new hook doesn't even load the call frame unless you give it a forced render in dev mode? But it didn't happen when I tried on codesandbox so, I think this is a very specific problem in our workspace; but I do believe Daily has something to do with it.
If it's not too much for you or any member of the team to check our codebase and see what is the root cause of these issues:

  • Initial load of the iframe causes a flashlight effect in dark mode when used with theme providers.
  • Iframe origin issues.
  • useCallFrame not creating an instance.

If there is an actual problem and not some newbie mistake, it could be a very niche Daily bug that we might find.


Edit: I figured out not creating call frame instance problem. Using dynamic import causes it.

@ahkhanjani
Copy link
Author

Alternatively we could invite you for a session in our classroom to see the problem in action. Our product is up and running. Anything that helps spot the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants