Skip to content

fix(compass-components): improve dark theme toggle appearance COMPASS-5470 #2751

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Feb 6, 2022
Merged

fix(compass-components): improve dark theme toggle appearance COMPASS-5470 #2751

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3dda3fd
Make toggle use dark theme from context
Anemy Feb 1, 2022
f1bee25
Make theme apply to connect form with initial value
Anemy Feb 1, 2022
5163c31
remove extra changes in workspace
Anemy Feb 1, 2022
b19d81c
Add event tests
Anemy Feb 2, 2022
bccd0c6
remove comment
Anemy Feb 2, 2022
dad2a58
merge main
Anemy Feb 2, 2022
6591e0e
Move theme provider to wrap all of compass-home, enum instead of stri…
Anemy Feb 3, 2022
346d632
Move theme setting into seperate file
Anemy Feb 3, 2022
6890316
merge main
Anemy Feb 3, 2022
d3f8eca
update deps
Anemy Feb 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 79 additions & 18 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 14 additions & 0 deletions packages/compass-components/src/components/toggle.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import React from 'react';
import LeafyGreenToggle from '@leafygreen-ui/toggle';

import { Theme, useTheme } from '../hooks/use-theme';

function Toggle(
props: React.ComponentProps<typeof LeafyGreenToggle>
Copy link
Collaborator

@gribnoysup gribnoysup Feb 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to to be overridable like that? What should take precedence?

Suggested change
props: React.ComponentProps<typeof LeafyGreenToggle>
props: Omit<React.ComponentProps<typeof LeafyGreenToggle>, 'darkMode'>

Copy link
Member Author

@Anemy Anemy Feb 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I think we probably want it to be overrideable, yes, but I'm alright with either approach. If it's a component which always is displayed in a darkMode or always needs to be light mode I think we'll want to customize it and override the provider. I think we could also add more providers in that situation, but if it's just one component I think it's simpler to just override this. I'm cool with either here - is there one you prefer?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this makes sense! Let's keep it like that for now 👍

): ReturnType<typeof LeafyGreenToggle> {
const theme = useTheme();

return <LeafyGreenToggle darkMode={theme?.theme === Theme.Dark} {...props} />;
}
Comment on lines +6 to +12
Copy link
Collaborator

@gribnoysup gribnoysup Feb 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raised this with leafygreen, if that's the only way to set the darkMode for their components, we are at the risk of wrapping every single leafygreen component that we export here which sounds like a giant maintenance headache

Copy link
Collaborator

@mcasimir mcasimir Feb 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it would be better if they'd have a provider we can use, I'd expect LG to also provide themes and guidelines for the palette, which is something we'd have to do on our side otherwise:

const darkTheme = {
    theme: Theme.Dark
    colors: {
        accent: palette.green.light1,
        // .. etc
    }
}

Copy link
Collaborator

@mcasimir mcasimir Feb 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do something like this before re-exporting the components on our side:

const LGComponentWithTheme<T>(Wrappee: T): T {
   const theme = useTheme()
   return (props) => Wrappee(...props, {...darkmode: theme?.theme})
}

const Toggle = LGComponentWithTheme(Toggle)
export Toggle;

Copy link
Collaborator

@gribnoysup gribnoysup Feb 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, yeah, we totally can do something like that, HOC is indeed a good pattern to use here. It's just weird that we have to even though we all agree that we want to do as little changes to leafygreen as possible

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lg team said this will be addressed when they are eventually working on a dark mode support

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should we do for now? I think we probably won't need to do many fixes like this until dark theme/the new provider is released in leafygreen so having one component listening to the theme is probably alright?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would merge this as it is


export { Toggle };
32 changes: 32 additions & 0 deletions packages/compass-components/src/hooks/use-theme.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import React, { createContext, useContext } from 'react';

export enum Theme {
Light = 'Light',
Dark = 'Dark',
}

type ThemeState = {
theme: Theme;
};

const ThemeContext = createContext<ThemeState>({
theme: Theme.Light,
});

const ThemeProvider = ({
children,
theme,
}: {
children: React.ReactChildren;
theme: ThemeState;
}): React.ReactElement => {
return (
<ThemeContext.Provider value={theme}>{children}</ThemeContext.Provider>
);
};

function useTheme(): ThemeState {
return useContext(ThemeContext);
}

export { useTheme, ThemeProvider };
4 changes: 3 additions & 1 deletion packages/compass-components/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ export {
default as Toast,
Variant as ToastVariant,
} from '@leafygreen-ui/toast';
export { default as Toggle } from '@leafygreen-ui/toggle';
export { Toggle } from './components/toggle';

export { breakpoints, spacing } from '@leafygreen-ui/tokens';
export { Tooltip } from './components/tooltip';
export {
Expand Down Expand Up @@ -92,6 +93,7 @@ export {
useHoverState,
FocusState,
} from './hooks/use-focus-hover';
export { useTheme, Theme, ThemeProvider } from './hooks/use-theme';
export {
ContentWithFallback,
FadeInPlaceholder,
Expand Down
4 changes: 4 additions & 0 deletions packages/compass-home/src/components/home.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,8 @@ describe('Home [Component]', function () {
'open-instance-workspace',
'open-namespace-in-new-tab',
'all-collection-tabs-closed',
'darkmode-enable',
'darkmode-disable',
];

events.forEach((name) => {
Expand Down Expand Up @@ -242,6 +244,8 @@ describe('Home [Component]', function () {
'open-instance-workspace',
'open-namespace-in-new-tab',
'all-collection-tabs-closed',
'darkmode-enable',
'darkmode-disable',
];

events.forEach((name) => {
Expand Down
60 changes: 54 additions & 6 deletions packages/compass-home/src/components/home.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
import React, { useCallback, useEffect, useReducer, useRef } from 'react';
import { css } from '@mongodb-js/compass-components';
import React, {
useCallback,
useEffect,
useReducer,
useRef,
useState,
} from 'react';
import { ThemeProvider, css } from '@mongodb-js/compass-components';
import { Theme } from '@mongodb-js/compass-components';
import { getConnectionTitle } from 'mongodb-data-service';
import type { ConnectionInfo, DataService } from 'mongodb-data-service';
import toNS from 'mongodb-ns';
Expand Down Expand Up @@ -93,7 +100,9 @@ function Home({ appName }: { appName: string }): React.ReactElement | null {

const [{ connectionTitle, isConnected, namespace }, dispatch] = useReducer(
reducer,
{ ...initialState }
{
...initialState,
}
);

function onDataServiceConnected(
Expand Down Expand Up @@ -203,7 +212,6 @@ function Home({ appName }: { appName: string }): React.ReactElement | null {
};
}
}, [appRegistry, showNewConnectForm, onDataServiceDisconnected]);

useEffect(() => {
// Setup app registry listeners.
appRegistry.on('data-service-connected', onDataServiceConnected);
Expand Down Expand Up @@ -266,6 +274,46 @@ function Home({ appName }: { appName: string }): React.ReactElement | null {
);
}

Home.displayName = 'HomeComponent';
function ThemedHome(
props: React.ComponentProps<typeof Home>
): ReturnType<typeof Home> {
const appRegistry = useAppRegistryContext();

const [theme, setTheme] = useState<Theme>({
theme: (global as any).hadronApp?.theme ?? Theme.Light,
});

function onDarkModeEnabled() {
setTheme({
theme: Theme.Dark,
});
}

function onDarkModeDisabled() {
setTheme({
theme: Theme.Light,
});
}

useEffect(() => {
// Setup app registry listeners.
appRegistry.on('darkmode-enable', onDarkModeEnabled);
appRegistry.on('darkmode-disable', onDarkModeDisabled);

return () => {
// Clean up the app registry listeners.
appRegistry.removeListener('darkmode-enable', onDarkModeEnabled);
appRegistry.removeListener('darkmode-disable', onDarkModeDisabled);
};
}, [appRegistry]);

return (
<ThemeProvider theme={theme}>
<Home {...props}></Home>
</ThemeProvider>
);
}

ThemedHome.displayName = 'HomeComponent';

export default Home;
export default ThemedHome;
1 change: 1 addition & 0 deletions packages/compass/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@
"email": "compass@mongodb.com"
},
"dependencies": {
"@mongodb-js/compass-components": "^0.10.0",
"@mongosh/node-runtime-worker-thread": "^1.1.9",
"kerberos": "^2.0.0-beta.0",
"keytar": "^7.7.0",
Expand Down
Loading