VulcanJS / Vulcan

🌋 A toolkit to quickly build apps with React, GraphQL & Meteor
http://vulcanjs.org
MIT License
7.98k stars 1.89k forks source link

Unify Server and Client ThemeProviders and pass apolloClient as prop #2691

Closed Apollinaire closed 3 years ago

Apollinaire commented 3 years ago

Also pass context on serverside.

The Client version of wrapWithMuiTheme was using Components.ThemeProvider while the server version was using the default ThemeProvider, so you could overwrite it client-side but not server-side. I introduced this three years ago in a PR to ErikDakoda's package).

Now both the server and the client ThemeProvider uses the Components system, so they can be overwritten. The implementation for the default ThemeProvider was the same on client and server, so I implemented it directly in the components/theme folder which is common to both server and client environments. Also, I add the apolloClient prop that lets you query data right inside the ThemeProvider to be able to have a dynamic theme depending on a graphql query. We need it because the ThemeProvider is higher than the ApolloClientProvider in the react tree, so a useQuery would not have the context for it.

Here is an example of dynamic ThemeProvider:

const CustomThemeProvider = ({ children, apolloClient }) => {
  const { data } = useQuery(currentThemeQuery, { client: apolloClient });
  const primaryColor = data?.currentTheme?.primaryColor || DEFAULT_PRIMARY_COLOR;
  const secondaryColor = data?.currentTheme?.secondaryColor || DEFAULT_SECONDARY_COLOR;

  const memoizedTheme = useMemo(() => {
    return createTheme({ primaryColor, secondaryColor });
  }, [primaryColor, secondaryColor]);

  return (
    <ThemeProvider theme={memoizedTheme}>
      <JssCleanup>{children}</JssCleanup>
    </ThemeProvider>
  );
};
eric-burel commented 3 years ago

Sounds good, but why is the theme provider higher than Apollo provider in the first place?

ErikDakoda commented 3 years ago

Sounds good, but why is the theme provider higher than Apollo provider in the first place?

That's a good question. It's probably worth trying to switch their order - if that's reasonably easy.

Apollinaire commented 3 years ago

I have been thinking about how to offer a light/dark theme toggle

That was my use case three years ago when I submitted a PR to you, had it working, but the theme we had was filled with hardcoded colors so dark version was ugly and unreadable.

why is the theme provider higher than Apollo provider in the first place?

I'll get a try at this

Apollinaire commented 3 years ago

why is the theme provider higher than Apollo provider in the first place?

I think it's because it's more convienient to do it that way in the server-render. I managed to invert that easily for the client render, but for the server render we do two "passes" on the React tree: one to get the initial Apollo Cache, and one to do the actual HTML render.

I'm not confident enough to touch this, and I think that for this PR, we can stick to the minor modifications that I initially submitted.

eric-burel commented 3 years ago

Fine then, it lacks some testing beforehand indeed

Apollinaire commented 3 years ago

@ErikDakoda I made the changes you asked for, tell me if that fits your needs!