Skip to content
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

fix: remove selector from theming #3879

Merged
merged 4 commits into from
Jan 30, 2025
Merged

fix: remove selector from theming #3879

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4dd85f2
fix: remove selector from theming
tjuanitas Jan 29, 2025
fc02e17
fix: prevent unnecessary insert
tjuanitas Jan 29, 2025
aee3d02
Merge branch 'master' into fix-remove-theme-selector
tjuanitas Jan 30, 2025
c891bf1
Merge branch 'master' into fix-remove-theme-selector
mergify[bot] Jan 30, 2025
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
4 changes: 2 additions & 2 deletions src/elements/common/theming/ThemingStyles.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import useCustomTheming from './useCustomTheming';
import { ThemingProps } from './types';

const ThemingStyles = ({ selector, theme }: ThemingProps) => {
useCustomTheming({ selector, theme });
const ThemingStyles = ({ theme }: ThemingProps) => {
useCustomTheming({ theme });

return null;
};
Expand Down
1 change: 0 additions & 1 deletion src/elements/common/theming/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,5 @@ export interface Theme {
}

export interface ThemingProps {
selector?: string;
theme?: Theme;
}
10 changes: 7 additions & 3 deletions src/elements/common/theming/useCustomTheming.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { useEffect } from 'react';
import { convertTokensToCustomProperties } from './utils';
import { ThemingProps } from './types';

const useCustomTheming = ({ selector, theme = {} }: ThemingProps) => {
const useCustomTheming = ({ theme = {} }: ThemingProps) => {
const { tokens } = theme;

const customProperties = convertTokensToCustomProperties(tokens);
Expand All @@ -11,15 +11,19 @@ const useCustomTheming = ({ selector, theme = {} }: ThemingProps) => {
.join(';');

useEffect(() => {
if (!styles) {
return undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't return anything here for useEffect. We would want to just end execution with return; right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes! I wanted to do that but there's an eslint rule about consistent returns: https://eslint.org/docs/latest/rules/consistent-return#treatundefinedasunspecified

basically return; (implicit undefined) doesn't work but return undefined; (explicit) does work 🤷

https://stackoverflow.com/questions/67658900/arrow-function-expected-no-return-value-with-clean-up-function-in-useeffect

}

const styleEl = document.createElement('style');
document.head.appendChild(styleEl);

styleEl.sheet.insertRule(`${selector ?? ':root'} { ${styles} }`);
styleEl.sheet.insertRule(`:root { ${styles} }`);

return () => {
document.head.removeChild(styleEl);
};
}, [selector, styles]);
}, [styles]);
};

export default useCustomTheming;
2 changes: 1 addition & 1 deletion src/elements/content-explorer/ContentExplorer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1665,7 +1665,7 @@ class ContentExplorer extends Component<Props, State> {
return (
<Internationalize language={language} messages={messages}>
<div id={this.id} className={styleClassName} ref={measureRef} data-testid="content-explorer">
<ThemingStyles selector={`#${this.id}`} theme={theme} />
<ThemingStyles theme={theme} />
<div className="be-app-element" onKeyDown={this.onKeyDown} tabIndex={0}>
{!isDefaultViewMetadata && (
<>
Expand Down
2 changes: 1 addition & 1 deletion src/elements/content-picker/ContentPicker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1240,7 +1240,7 @@ class ContentPicker extends Component<Props, State> {
return (
<Internationalize language={language} messages={messages}>
<div id={this.id} className={styleClassName} ref={measureRef} data-testid="content-picker">
<ThemingStyles selector={`#${this.id}`} theme={theme} />
<ThemingStyles theme={theme} />
<div className="be-app-element" onKeyDown={this.onKeyDown} tabIndex={0}>
<Header
view={view}
Expand Down
2 changes: 1 addition & 1 deletion src/elements/content-preview/ContentPreview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1332,7 +1332,7 @@ class ContentPreview extends React.PureComponent<Props, State> {
onKeyDown={this.onKeyDown}
tabIndex={0}
>
<ThemingStyles selector={`#${this.id}`} theme={theme} />
<ThemingStyles theme={theme} />
{hasHeader && (
<PreviewHeader
file={file}
Expand Down
2 changes: 1 addition & 1 deletion src/elements/content-sidebar/Sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ class Sidebar extends React.Component<Props, State> {

return (
<aside id={this.id} className={styleClassName} data-testid="preview-sidebar">
<ThemingStyles selector={`#${this.id}`} theme={theme} />
<ThemingStyles theme={theme} />
{isLoading ? (
<div className="bcs-loading">
<LoadingIndicator />
Expand Down
4 changes: 2 additions & 2 deletions src/elements/content-uploader/ContentUploader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1272,7 +1272,7 @@ class ContentUploader extends Component<ContentUploaderProps, State> {
<TooltipProvider>
{useUploadsManager ? (
<div ref={measureRef} className={styleClassName} id={this.id}>
<ThemingStyles selector={`#${this.id}`} theme={theme} />
<ThemingStyles theme={theme} />
<UploadsManager
isDragging={isDraggingItemsToUploadsManager}
isExpanded={isUploadsManagerExpanded}
Expand All @@ -1289,7 +1289,7 @@ class ContentUploader extends Component<ContentUploaderProps, State> {
</div>
) : (
<div ref={measureRef} className={styleClassName} id={this.id}>
<ThemingStyles selector={`#${this.id}`} theme={theme} />
<ThemingStyles theme={theme} />
<DroppableContent
addDataTransferItemsToUploadQueue={this.addDroppedItemsToUploadQueue}
addFiles={this.addFilesToUploadQueue}
Expand Down