Provide a "themeChange" event and "currentTheme" property in LensRendererExtension

stefcameron commented 3 years ago

Right now, the only way to detect a change to the Lens theme in the JS layer is to use a MutationObserver to observe the <body> element's class attribute to see if the "theme-light" class gets added to it. If it's there, then the theme is LIGHT; otherwise, consider it DARK.

Right now, this requires a lot of boilerplate:

//
// page.tsx
//

import { ThemeProvider } from 'emotion-theming';
import { lightTheme, darkTheme } from './theme';

const lightThemeClassName = 'theme-light';

export const MyPage = function () {
  //
  // STATE
  //

  const [theme, setTheme] = useState(
    document.body.classList.contains(lightThemeClassName)
      ? lightTheme
      : darkTheme
  );

  //
  // EFFECTS
  //

  useEffect(function () {
    const observer = new MutationObserver(function (mutations: MutationRecord[]) {
      mutations.forEach((mutation: MutationRecord) => {
        if (mutation.type === 'attributes' && mutation.attributeName === 'class') {
          if ((mutation.target as HTMLElement).classList.contains(lightThemeClassName)) {
            setTheme(lightTheme);
          } else {
            setTheme(darkTheme);
          }
        }
      });
    });

    observer.observe(document.body, {
      attributes: true,
      attributeFilter: ['class'],
    });

    return function () {
      observer.disconnect();
    };
  }, []); // run once on mount

  //
  // RENDER
  //

  return (
    <ThemeProvider theme={theme}>
      <View />
    </ThemeProvider>
  );
};

The requirement is to trigger a re-render whenever the theme changes, so some type of function call is needed.

We could handle this more elegantly/formally if the LensRendererExtension provided events that one could subscribe to, like a 'themeChange' event, as well as a property identifying the current theme:

//
// renderer.tsx
//

export default class ExtensionRenderer extends LensRendererExtension {
  globalPages = [
    {
      ...addRoute,
      url: getAddUrl(),
      hideInMenu: true,
      components: {
        Page: () => <MyPage extension={this} />,
      },
    },
  ];
}

//
// page.tsx
//

import { ThemeProvider } from 'emotion-theming';
import { lightTheme, darkTheme } from './theme';

export const MyPage = function ({ extension }) {
  //
  // STATE
  //

  const [theme, setTheme] = useState(
    extension.currentTheme === 'light' ? lightTheme : darkTheme
  );

  //
  // EFFECTS
  //

  useEffect(function () {
    const handler = (name: string) => setTheme(name === 'light' ? lightTheme : darkTheme);
    extension.addEventListener('themeChange', handler);

    return function () {
      extension.removeEventListener('themeChange', handler);
    };
  }, []); // run once on mount

  //
  // RENDER
  //

  return (
    <ThemeProvider theme={theme}>
      <View />
    </ThemeProvider>
  );
};

Lens could also pass-in an entire theme object if it wanted to, instead of just the name, but _make sure you have a mode or name property on that object so that it's possible to know what "mode" the theme is...

aleksfront commented 3 years ago

@stefcameron We don't need such a boilerplate for now. I will provide an example in styling samples (https://github.com/lensapp/lens/pull/1371). After https://github.com/lensapp/lens/pull/1371 will be merged and new types published, any extension will be able to get active theme from @k8slens/extensions:

import { Theme } from "@k8slens/extensions";

export class SampleExtension extends React.Component {
  render() {
    return (
      <div>{Theme.getActiveTheme().name}<div>
    );
  }
}

getActiveTheme provides all metadata which is described here https://github.com/lensapp/lens/blob/master/src/renderer/theme.store.ts#L13

stefcameron commented 3 years ago

@aleksfront Does this satisfy the theme switching requirement/use case I'm mainly addressing here? When I look at the Emotion sample in https://github.com/lensapp/lens-extension-samples/pull/7, I don't see theme switching there. I see CSS-in-JS being use to style a component with some theme-based CSS variables (i.e. variables whose values will change when the Lens theme changes), but I don't see an example of subscribing to Lens to be notified when the theme changes.

Providing Theme.getActiveTheme().name satisfies one of the requirements: For any given render, find out what theme Lens is currently using.

The other (more important) requirement is to trigger a render when Lens changes its theme. For that to happen, we need a function to be called by Lens (i.e. an event handler), to which a component can subscribe (typically the main "app" renderer/page component) to be notified, to then call Theme.getActiveTheme() and then pass that into the <ThemeProvider theme={newTheme} />, thus triggering that re-render.

When you change the theme in the Lens settings, it does not always trigger a re-render.

aleksfront commented 3 years ago

@stefcameron There is a way to catch such a theme change event inside your component using observer functoin/decorator from mobx and mobx-react . Such dependencies are already injected into the extensions.

Example:

import React from "react"
import { observer } from "mobx-react"
import { App, Component, Theme } from "@k8slens/extensions";

@observer
export class SupportPage extends React.Component {
  render() {
    return (
      <div className="SupportPage">
        <h1>Active theme is {Theme.getActiveTheme().name}</h1>
      </div>
    );
  }
}

But there's probably no need for such an actions since theme currently gets changed with Preferences page only. So when you switch the theme, app navigates away from Preferences and your component gets rerendered. But provided example is relevant for theoretical case when theme is switched by some explicit action without moving into /preferences.

Resources: https://mobx.js.org/README.html https://github.com/mobxjs/mobx-react#observercomponent https://mobx.js.org/react-integration.html

stefcameron commented 3 years ago

There is a way to catch such a theme change event inside your component using observer functoin/decorator from mobx and mobx-react . Such dependencies are already injected into the extensions.

Aha! The Lens team really needs to document about this mobx stuff. I think @Nokel81 was going to write something up. I don't know anything about mobx or how it works, never mind the fact it's provided as a global! 😄

Thanks for the mobx docs links -- especially the one about ObserverComponent since it shows me how to use this with a functional component. Lens docs tend to favor classical components; I almost never use those anymore.

But there's probably no need for such an actions since theme currently gets changed with Preferences page only. So when you switch the theme, app navigates away from Preferences and your component gets rerendered.

That's not the case if you're depending on JavaScript code to pick a theme object, which is typically the case when using CSS-in-JS, or when you have to hard-code color values for the custom scroll bars because they aren't provided as themed CSS variables by Lens... (plug for #1337 ). If you're just relying on CSS, then yes, it gets re-rendered and you're fine. But if you need to choose between one theme object or another, when you re-render, you'll still choose the old theme object. You need that hook to know when things change.

Nokel81 commented 3 years ago

Yes I plan on working on those docs today once I finish up with my previous PRs.

aleksfront commented 3 years ago

I guess we can close this issue right after improving the docs and merge described samples https://github.com/lensapp/lens-extension-samples/pull/7

stefcameron commented 3 years ago

Seems like @k8slens/extensions does not export the Theme type, at least not in beta.2. The typings don't include anything related to the Theme object that I can see.

stefcameron commented 3 years ago

I guess we can close this issue right after improving the docs and merge described samples lensapp/lens-extension-samples#7

Those are great samples, but I feel they're missing theme switching by monitoring changes to the theme object. They all rely on CSS variable changes for theme updates, but that's not always adequate.

stefcameron commented 3 years ago

Seems like @k8slens/extensions does not export the Theme type, at least not in beta.2. The typings don't include anything related to the Theme object that I can see.

Looks like it's coming in beta.3 based on release indication on #1371

lensapp / lens

Provide a "themeChange" event and "currentTheme" property in LensRendererExtension #1336