daily-co / daily-js

https://docs.daily.co/reference/daily-js
BSD 2-Clause "Simplified" License
103 stars 33 forks source link

Switch between light and dark themes in Prebuilt UI #231

Open ahkhanjani opened 12 months ago

ahkhanjani commented 12 months ago

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} />;
}
Regaddi commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

@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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

Regaddi commented 9 months ago

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 commented 9 months ago

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 commented 8 months ago

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:

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 commented 8 months ago

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.