Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Commit ce8679b

Browse files
authored
Remove ThemeProps and add new theme state logic (#47864)
## Before Before this PR we had theme state on the top level in the Layout.tsx, we had a hook that provided an object with public API for theme (to read or to set a new value), there were at least two problems - Complex implementation of useThemeProps (rxjs to listen browser theme, mixed with logic around setting theme CSS classes) - Prop drilling, after we created these theme API on the top level, it was distributed via props I think I don't need to dive deep in why prop drilling is a bad practice, but still, with prop drilling and nested components, we were updating the whole react tree when theme was changed, also passing these public Theme props required extending your component props with special type `ThemeProps` and this becomes a mess quickly if you extend something else (like one component props extended other component props) ## Now In this PR, we use more standard ways for global state distirubiotion - Context. In the new `useTheme` hook, we don't do any effects with CSS classes (consumer on the top level should handle it), so it's safe to call useTheme on any UI level. Privosly it wasn't safe, and because of this, we had this problem https://github.com/sourcegraph/sourcegraph/issues/47501 ## Further steps Ideally we shouldn't have any JS logic about theme besides CSS classes set on the document element. And all colors should work through CSS variables *custom-properties, but at the moment, we have at least two major places where we have to have JS check about theme. - Brand logo (since we use svg as image, in img src attribute), we should change URL based on theme (in dark theme brand logo has difference color value). I'm wondering that we can probably change it and use svg directly without img tag) - Monaco editor doesn't support colors through CSS variables, and we change set of colors in js runtime based on theme value. I don't know maybe with CodeMirror, we could use just CSS variables and remove this logic
1 parent 3083954 commit ce8679b

File tree

171 files changed

+833
-1201
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

171 files changed

+833
-1201
lines changed

client/branded/src/components/panel/TabbedPanelContent.fixtures.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ export const panels: Panel[] = [
2525
export const panelProps: React.ComponentPropsWithoutRef<typeof TabbedPanelContent> = {
2626
repoName: 'git://github.com/foo/bar',
2727
fetchHighlightedFileLineRanges: () => of([]),
28-
isLightTheme: true,
2928
platformContext: {} as any,
3029
settingsCascade: { subjects: null, final: null },
3130
telemetryService: NOOP_TELEMETRY_SERVICE,

client/branded/src/components/panel/TabbedPanelContent.tsx

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import { FetchFileParameters } from '@sourcegraph/shared/src/backend/file'
1010
import { PlatformContextProps } from '@sourcegraph/shared/src/platform/context'
1111
import { SettingsCascadeProps } from '@sourcegraph/shared/src/settings/settings'
1212
import { TelemetryProps } from '@sourcegraph/shared/src/telemetry/telemetryService'
13-
import { ThemeProps } from '@sourcegraph/shared/src/theme'
1413
import {
1514
Button,
1615
useObservable,
@@ -29,7 +28,7 @@ import { EmptyPanelView } from './views/EmptyPanelView'
2928

3029
import styles from './TabbedPanelContent.module.scss'
3130

32-
interface TabbedPanelContentProps extends PlatformContextProps, SettingsCascadeProps, TelemetryProps, ThemeProps {
31+
interface TabbedPanelContentProps extends PlatformContextProps, SettingsCascadeProps, TelemetryProps {
3332
repoName?: string
3433
fetchHighlightedFileLineRanges: (parameters: FetchFileParameters, force?: boolean) => Observable<string[][]>
3534
}

client/branded/src/search-ui/input/CodeMirrorQueryInput.tsx

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ import { Filter } from '@sourcegraph/shared/src/search/query/token'
2828
import { appendContextFilter } from '@sourcegraph/shared/src/search/query/transformer'
2929
import { fetchStreamSuggestions as defaultFetchStreamSuggestions } from '@sourcegraph/shared/src/search/suggestions'
3030
import { RecentSearch } from '@sourcegraph/shared/src/settings/temporary/recentSearches'
31-
import { ThemeProps } from '@sourcegraph/shared/src/theme'
31+
import { useIsLightTheme } from '@sourcegraph/shared/src/theme'
3232
import { isInputElement } from '@sourcegraph/shared/src/util/dom'
3333

3434
import { createDefaultSuggestions, singleLine } from './codemirror'
@@ -93,7 +93,6 @@ export const CodeMirrorMonacoFacade: React.FunctionComponent<CodeMirrorQueryInpu
9393
globbing,
9494
onEditorCreated,
9595
interpretComments,
96-
isLightTheme,
9796
className,
9897
preventNewLine = true,
9998
placeholder,
@@ -296,7 +295,6 @@ export const CodeMirrorMonacoFacade: React.FunctionComponent<CodeMirrorQueryInpu
296295
return (
297296
<>
298297
<CodeMirrorQueryInput
299-
isLightTheme={isLightTheme}
300298
onEditorCreated={editorCreated}
301299
patternType={patternType}
302300
interpretComments={interpretComments}
@@ -313,7 +311,7 @@ export const CodeMirrorMonacoFacade: React.FunctionComponent<CodeMirrorQueryInpu
313311

314312
const EMPTY: any[] = []
315313

316-
interface CodeMirrorQueryInputProps extends ThemeProps, SearchPatternTypeProps {
314+
interface CodeMirrorQueryInputProps extends SearchPatternTypeProps {
317315
value: string
318316
onEditorCreated?: (editor: EditorView) => void
319317
// Whether comments are parsed and highlighted
@@ -327,12 +325,14 @@ interface CodeMirrorQueryInputProps extends ThemeProps, SearchPatternTypeProps {
327325
* theming, syntax highlighting and token info.
328326
*/
329327
export const CodeMirrorQueryInput: React.FunctionComponent<CodeMirrorQueryInputProps> = React.memo(
330-
({ isLightTheme, onEditorCreated, patternType, interpretComments, value, className, extensions = EMPTY }) => {
328+
({ onEditorCreated, patternType, interpretComments, value, className, extensions = EMPTY }) => {
331329
// This is using state instead of a ref because `useRef` doesn't cause a
332330
// re-render when the ref is attached, but we need that so that
333331
// `useCodeMirror` is called again and the editor is actually created.
334332
// See https://reactjs.org/docs/hooks-faq.html#how-can-i-measure-a-dom-node
335333
const [container, setContainer] = useState<HTMLDivElement | null>(null)
334+
const isLightTheme = useIsLightTheme()
335+
336336
const externalExtensions = useMemo(() => new Compartment(), [])
337337
const themeExtension = useMemo(() => new Compartment(), [])
338338

client/branded/src/search-ui/input/QueryInput.ts

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,16 +5,14 @@ import {
55
SearchPatternTypeProps,
66
} from '@sourcegraph/shared/src/search'
77
import { fetchStreamSuggestions as defaultFetchStreamSuggestions } from '@sourcegraph/shared/src/search/suggestions'
8-
import { ThemeProps } from '@sourcegraph/shared/src/theme'
98

109
import { IEditor } from './LazyQueryInput'
1110

1211
/**
1312
* Props that the Monaco and CodeMirror implementation have in common.
1413
*/
1514
export interface QueryInputProps
16-
extends ThemeProps,
17-
Pick<CaseSensitivityProps, 'caseSensitive'>,
15+
extends Pick<CaseSensitivityProps, 'caseSensitive'>,
1816
SearchPatternTypeProps,
1917
Pick<SearchContextProps, 'selectedSearchContextSpec'> {
2018
isSourcegraphDotCom: boolean // Needed for query suggestions to give different options on dotcom; see SOURCEGRAPH_DOT_COM_REPO_COMPLETION

client/branded/src/search-ui/input/SearchBox.story.tsx

Lines changed: 5 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@ const defaultProps: SearchBoxProps = {
2828
final: null,
2929
subjects: null,
3030
},
31-
isLightTheme: false,
3231
globbing: false,
3332
queryState: { query: 'hello repo:test' },
3433
isSourcegraphDotCom: false,
@@ -57,48 +56,34 @@ export const SearchBoxStory: Story = () => (
5756
<div>
5857
<H2>Default</H2>
5958
<div className="w-100 d-flex my-2">
60-
<SearchBox {...defaultProps} isLightTheme={props.isLightTheme} />
59+
<SearchBox {...defaultProps} />
6160
</div>
6261

6362
<H2>Regexp enabled</H2>
6463
<div className="w-100 d-flex my-2">
65-
<SearchBox
66-
{...defaultProps}
67-
patternType={SearchPatternType.regexp}
68-
isLightTheme={props.isLightTheme}
69-
/>
64+
<SearchBox {...defaultProps} patternType={SearchPatternType.regexp} />
7065
</div>
7166

7267
<H2>Structural enabled</H2>
7368
<div className="w-100 d-flex my-2">
74-
<SearchBox
75-
{...defaultProps}
76-
patternType={SearchPatternType.structural}
77-
isLightTheme={props.isLightTheme}
78-
/>
69+
<SearchBox {...defaultProps} patternType={SearchPatternType.structural} />
7970
</div>
8071

8172
<H2>Case sensitivity enabled</H2>
8273
<div className="w-100 d-flex my-2">
83-
<SearchBox {...defaultProps} caseSensitive={true} isLightTheme={props.isLightTheme} />
74+
<SearchBox {...defaultProps} caseSensitive={true} />
8475
</div>
8576

8677
<H2>With search contexts</H2>
8778
<div className="w-100 d-flex my-2">
88-
<SearchBox
89-
{...defaultProps}
90-
showSearchContext={true}
91-
isLightTheme={props.isLightTheme}
92-
selectedSearchContextSpec="global"
93-
/>
79+
<SearchBox {...defaultProps} showSearchContext={true} selectedSearchContextSpec="global" />
9480
</div>
9581

9682
<H2>With search contexts, user context selected</H2>
9783
<div className="w-100 d-flex my-2">
9884
<SearchBox
9985
{...defaultProps}
10086
showSearchContext={true}
101-
isLightTheme={props.isLightTheme}
10287
selectedSearchContextSpec="@username/test-version-1.5"
10388
/>
10489
</div>
@@ -108,7 +93,6 @@ export const SearchBoxStory: Story = () => (
10893
<SearchBox
10994
{...defaultProps}
11095
showSearchContext={true}
111-
isLightTheme={props.isLightTheme}
11296
queryState={{ query: 'hello context:global' }}
11397
selectedSearchContextSpec="@username"
11498
/>

client/branded/src/search-ui/input/SearchBox.tsx

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import { fetchStreamSuggestions as defaultFetchStreamSuggestions } from '@source
1111
import { RecentSearch } from '@sourcegraph/shared/src/settings/temporary/recentSearches'
1212
import { useTemporarySetting } from '@sourcegraph/shared/src/settings/temporary/useTemporarySetting'
1313
import { TelemetryProps } from '@sourcegraph/shared/src/telemetry/telemetryService'
14-
import { ThemeProps } from '@sourcegraph/shared/src/theme'
1514

1615
import { IEditor, LazyQueryInput, LazyQueryInputProps } from './LazyQueryInput'
1716
import { SearchButton } from './SearchButton'
@@ -24,7 +23,6 @@ import styles from './SearchBox.module.scss'
2423

2524
export interface SearchBoxProps
2625
extends Omit<TogglesProps, 'navbarSearchQuery' | 'submitSearch'>,
27-
ThemeProps,
2826
SearchContextInputProps,
2927
TelemetryProps,
3028
PlatformContextProps<'requestGraphQL'>,
@@ -185,7 +183,6 @@ export const SearchBox: FC<SearchBoxProps> = props => {
185183
fetchStreamSuggestions={props.fetchStreamSuggestions}
186184
globbing={props.globbing}
187185
interpretComments={props.interpretComments}
188-
isLightTheme={props.isLightTheme}
189186
isSourcegraphDotCom={props.isSourcegraphDotCom}
190187
onChange={props.onChange}
191188
onCompletionItemSelected={props.onCompletionItemSelected}

client/branded/src/search-ui/results/NoResultsPage.tsx

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import { QueryState, SearchContextProps } from '@sourcegraph/shared/src/search'
77
import { NoResultsSectionID as SectionID } from '@sourcegraph/shared/src/settings/temporary/searchSidebar'
88
import { useTemporarySetting } from '@sourcegraph/shared/src/settings/temporary/useTemporarySetting'
99
import { TelemetryProps } from '@sourcegraph/shared/src/telemetry/telemetryService'
10-
import { ThemeProps } from '@sourcegraph/shared/src/theme'
1110
import { Button, Link, Icon, H3, Text, H2 } from '@sourcegraph/wildcard'
1211

1312
import { QueryExamples } from '../components/QueryExamples'
@@ -44,7 +43,7 @@ const Container: React.FunctionComponent<React.PropsWithChildren<ContainerProps>
4443
</div>
4544
)
4645

47-
interface NoResultsPageProps extends ThemeProps, TelemetryProps, Pick<SearchContextProps, 'searchContextsEnabled'> {
46+
interface NoResultsPageProps extends TelemetryProps, Pick<SearchContextProps, 'searchContextsEnabled'> {
4847
isSourcegraphDotCom: boolean
4948
showSearchContext: boolean
5049
/** Available to web app through JS Context */
@@ -56,7 +55,6 @@ interface NoResultsPageProps extends ThemeProps, TelemetryProps, Pick<SearchCont
5655

5756
export const NoResultsPage: React.FunctionComponent<React.PropsWithChildren<NoResultsPageProps>> = ({
5857
searchContextsEnabled,
59-
isLightTheme,
6058
telemetryService,
6159
isSourcegraphDotCom,
6260
showSearchContext,

client/branded/src/search-ui/results/StreamingSearchResultsList.tsx

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import {
1919
} from '@sourcegraph/shared/src/search/stream'
2020
import { SettingsCascadeProps } from '@sourcegraph/shared/src/settings/settings'
2121
import { TelemetryProps } from '@sourcegraph/shared/src/telemetry/telemetryService'
22-
import { ThemeProps } from '@sourcegraph/shared/src/theme'
2322

2423
import { CommitSearchResult } from '../components/CommitSearchResult'
2524
import { FileContentSearchResult } from '../components/FileContentSearchResult'
@@ -37,8 +36,7 @@ import resultContainerStyles from '../components/ResultContainer.module.scss'
3736
import styles from './StreamingSearchResultsList.module.scss'
3837

3938
export interface StreamingSearchResultsListProps
40-
extends ThemeProps,
41-
SettingsCascadeProps,
39+
extends SettingsCascadeProps,
4240
TelemetryProps,
4341
Pick<SearchContextProps, 'searchContextsEnabled'>,
4442
PlatformContextProps<'requestGraphQL'> {
@@ -85,7 +83,6 @@ export const StreamingSearchResultsList: React.FunctionComponent<
8583
fetchHighlightedFileLineRanges,
8684
settingsCascade,
8785
telemetryService,
88-
isLightTheme,
8986
isSourcegraphDotCom,
9087
searchContextsEnabled,
9188
assetsRoot,
@@ -287,7 +284,6 @@ export const StreamingSearchResultsList: React.FunctionComponent<
287284
<NoResultsPage
288285
searchContextsEnabled={searchContextsEnabled}
289286
isSourcegraphDotCom={isSourcegraphDotCom}
290-
isLightTheme={isLightTheme}
291287
telemetryService={telemetryService}
292288
showSearchContext={searchContextsEnabled}
293289
assetsRoot={assetsRoot}

client/browser/src/browser-extension/ThemeWrapper.tsx

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,12 @@
11
import { useEffect, useMemo, useState } from 'react'
22

3-
import { ThemeProps } from '@sourcegraph/shared/src/theme'
4-
53
/**
64
* Wrapper for the browser extension that listens to changes of the OS theme.
75
*/
86
export function ThemeWrapper({
97
children,
108
}: {
11-
children: JSX.Element | null | ((props: ThemeProps) => JSX.Element | null)
9+
children: JSX.Element | null | ((props: { isLightTheme: boolean }) => JSX.Element | null)
1210
}): JSX.Element | null {
1311
const darkThemeMediaList = useMemo(() => window.matchMedia('(prefers-color-scheme: dark)'), [])
1412
const [isLightTheme, setIsLightTheme] = useState(!darkThemeMediaList.matches)

client/browser/src/browser-extension/after-install-page/AfterInstallPageContent.tsx

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,18 +5,18 @@ import classNames from 'classnames'
55

66
import { SourcegraphLogo } from '@sourcegraph/branded/src/components/SourcegraphLogo'
77
import { PhabricatorIcon } from '@sourcegraph/shared/src/components/icons'
8-
import { ThemeProps } from '@sourcegraph/shared/src/theme'
98
import { Link, Icon, Code, H1, H2, H3, Text } from '@sourcegraph/wildcard'
109

1110
import { getPlatformName } from '../../shared/util/context'
1211

1312
import styles from './AfterInstallPageContent.module.scss'
1413

15-
const Video: React.FunctionComponent<
16-
React.PropsWithChildren<
17-
{ name: string } & Pick<VideoHTMLAttributes<HTMLVideoElement>, 'width' | 'height'> & ThemeProps
18-
>
19-
> = ({ name, isLightTheme, width, height }) => {
14+
interface VideoProps extends Pick<VideoHTMLAttributes<HTMLVideoElement>, 'width' | 'height'> {
15+
name: string
16+
isLightTheme: boolean
17+
}
18+
19+
const Video: React.FC<VideoProps> = ({ name, isLightTheme, width, height }) => {
2020
const suffix = isLightTheme ? 'Light' : 'Dark'
2121
return (
2222
<video
@@ -43,7 +43,11 @@ const Video: React.FunctionComponent<
4343
)
4444
}
4545

46-
export const AfterInstallPageContent: React.FunctionComponent<React.PropsWithChildren<ThemeProps>> = props => {
46+
interface AfterInstallPageContentProps {
47+
isLightTheme: boolean
48+
}
49+
50+
export const AfterInstallPageContent: React.FC<AfterInstallPageContentProps> = props => {
4751
// Safari does not support the search shortcut. So don't show the feature.
4852
const isSafari = getPlatformName() === 'safari-extension'
4953
const showSearchShortcut = !isSafari

0 commit comments

Comments
 (0)