Skip to content

Terynk/theme designer code refactor #1

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

Open
terynk wants to merge 2 commits into
base: feature/theme-designer-refactor
Choose a base branch
from
Open

Terynk/theme designer code refactor #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
885613d
refactored code to improve usability and readability
terynkum Feb 14, 2025
7598712
Merge branch 'master' into terynk/theme-designer-code-refactor
terynk Feb 18, 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
142 changes: 94 additions & 48 deletions packages/react-components/theme-designer/src/components/Demo/Demo.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,34 +90,101 @@ const useStyles = makeStyles({
3) Note that the spinner was removed since it was causing confusing with the loading state of the page
Copy link

@mltejera mltejera Feb 18, 2025

Choose a reason for hiding this comment

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

You can probably remove these comments as well.

*/

export const Column1 = () => {
const AvatarSection = () => {
const styles = useStyles();
const dropdownId = useId('dropdown-default');
return (
<div className={styles.column}>
<div className={styles.avatar}>
<Persona
name="Cameron Evans"
secondaryText="Senior Researcher at Contoso"
avatar={{ color: 'brand', badge: { status: 'available' } }}
/>
</div>
<div className={styles.avatar}>
<Persona
name="Cameron Evans"
secondaryText="Senior Researcher at Contoso"
avatar={{ color: 'brand', badge: { status: 'available' } }}
/>
</div>
);
};

const TabSection = () => {
const tabValues = ['Home', 'Pages', 'Documents'];
return (
<TabList defaultSelectedValue="tab1">
<Tab value="tab1">Home</Tab>
<Tab value="tab2">Pages</Tab>
<Tab value="tab3">Documents</Tab>
{tabValues.map((tab, index) => (
Copy link

@mltejera mltejera Feb 18, 2025

Choose a reason for hiding this comment

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

Putting things in a loop is often the best choice. However, I think this is an instance where keeping it written out is probably the better path. Reason being it's less code and way more readable. Quick and easy reading is often more important in demo code than elegance.

<Tab key={index} value={`tab${index + 1}`}>{tab}</Tab>
))}
</TabList>
<Field>
<Input
placeholder="Find"
contentAfter={<Button aria-label="Find" appearance="transparent" icon={<SearchRegular />} size="small" />}
/>
);
};

const InputSection = () => {
const dropdownId = useId('dropdown-default');
const optionValues = ['Option 1', 'Option 2', 'Option 3'];
return (
<>
<Field>
<Input
placeholder="Find"
contentAfter={<Button aria-label="Find" appearance="transparent" icon={<SearchRegular />} size="small" />}
/>
</Field>
<Dropdown aria-labelledby={dropdownId} placeholder="Select" inlinePopup>
{optionValues.map((option, index) => (
<Option key={index} value={option}>
{option}
</Option>
))}
</Dropdown>
</>
)
};

const ControlRow = () => {
const styles = useStyles();
return (
<div className={styles.controlRow}>
<Button appearance="primary">Text</Button>
<div className={styles.controlColumn}>
<Switch defaultChecked={true} label="On" />
<Switch label="Off" />
</div>
</div>
);
};

const CheckboxSection = () => {
const styles = useStyles();
return (
<div className={styles.controlRow}>
<div className={styles.controlColumn}>
<Checkbox defaultChecked={true} label="Option 1" />
<Checkbox label="Option 2" />
</div>
<div className={styles.controlColumn}>
<RadioGroup>
<Radio defaultChecked={true} label="Option 1" />
<Radio label="Option 2" />
</RadioGroup>
</div>
</div>
);
};

const DescriptionSection = () => {
const styles = useStyles();
return (
<div className={styles.inputLabel}>
<Field label="Description" required>
<Input placeholder="Example Text" appearance="filled-darker" />
</Field>
<Dropdown aria-labelledby={dropdownId} placeholder="Select" inlinePopup>
<Option value="Action 1">Action 1</Option>
<Option value="Action 2">Action 2 </Option>
<Option value="Action 3">Action 3</Option>
</Dropdown>
</div>
);
};

export const Column1 = () => {
const styles = useStyles();
return (
<div className={styles.column}>
<AvatarSection />
<TabSection />
<InputSection />
</div>
);
};
Expand All @@ -126,26 +193,9 @@ export const Column2 = () => {
const styles = useStyles();
return (
<div className={styles.column}>
<div className={styles.controlRow}>
<Button appearance="primary">Text</Button>
<div className={styles.controlColumn}>
<Switch defaultChecked={true} label="On" />
<Switch label="Off" />
</div>
</div>
<ControlRow />
<Slider defaultValue={50} />
<div className={styles.controlRow}>
<div className={styles.controlColumn}>
<Checkbox defaultChecked={true} label="Option 1" />
<Checkbox label="Option 2" />
</div>
<div className={styles.controlColumn}>
<RadioGroup>
<Radio defaultChecked={true} label="Option 1" />
<Radio label="Option 2" />
</RadioGroup>
</div>
</div>
<CheckboxSection />
</div>
);
};
Expand All @@ -170,12 +220,8 @@ export const Column3 = () => {

return (
<div className={styles.column}>
<div className={styles.inputLabel}>
<Field label="Description" required>
<Input placeholder="Example Text" appearance="filled-darker" />
</Field>
</div>
<Link href="https://www.microsoft.com">Example link - www.microsoft.com</Link>
<DescriptionSection />
<Link href="https://www.microsoft.com">Example link - www.microsoft.com</Link>
</div>
);
};
Expand Down
166 changes: 90 additions & 76 deletions packages/react-components/theme-designer/src/components/Export/ExportPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,26 +46,85 @@ const useStyles = makeStyles({
height: '100%',
boxSizing: 'border-box',
},
panelContainer: {
zIndex: 100,
position: 'absolute',
top: '0px',
right: '0px',
width: '400px',
Copy link

@mltejera mltejera Feb 18, 2025

Choose a reason for hiding this comment

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

I know this is not your code, but in the interest of wax on, wax off....

I like to define file level semantic constants anytime a value gets to be significantly larger than the general tokens. And I often define them for things smaller than that as well.

This helps future readers understand where this "Big random number" came from, but more importantly that the decision was carefully considered and coordinated. The name leaves a breadcrumb trail of how to get there.

Copy link

@mltejera mltejera Feb 18, 2025

Choose a reason for hiding this comment

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

again, NYC, but same idea with z-indexes.

border: `1px solid ${tokens.colorNeutralStroke1}`,
borderRadius: tokens.borderRadiusXLarge,
backgroundColor: tokens.colorNeutralBackground1,
boxShadow: tokens.shadow64,
padding: '16px',
}
});

const ExportPanelHeader = ({ onClose }: { onClose: () => void }) => {
const styles = useStyles();
Copy link

@mltejera mltejera Feb 18, 2025

Choose a reason for hiding this comment

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

nit, nyc: I like to add the component name to the style function. This helps us share them clearly later on.

So this one would be useExportPanelHeaderStyles.

This is a habbit from writing shared library code that often ends up benefiting product vertical code.

return (
<div className={styles.exportHeader}>
<Text as="h1" id="headingID" size={500}>
Export Theme
</Text>
<Button size="small" appearance="subtle" icon={<DismissSquare24Regular />} onClick={onClose} />
</div>
);
};

const ExportPanelContent = ({
selectedValue,
onTabSelect,
exportedValue,
onClickCopyToClipboard,
}: {
selectedValue: TabValue;
onTabSelect: (event: SelectTabEvent, data: SelectTabData) => void;
exportedValue: string;
onClickCopyToClipboard: () => void;
}) => {
const styles = useStyles();
return (
<>
<Body1>
Passing this theme to a FluentProvider will automatically apply it to any Fluent components below it. You can
also export this to CodeSandbox with a few component examples below.
</Body1>
<br />
<TabList defaultSelectedValue="Code" selectedValue={selectedValue} onTabSelect={onTabSelect}>
<Tab value="Code">Code</Tab>
<Tab value="JSONLight">JSON (light)</Tab>
<Tab value="JSONDark">JSON (dark)</Tab>
</TabList>
<Textarea className={styles.text} size="small" value={exportedValue} id="textArea" textarea={{ className: styles.textarea }} readOnly />
<br />
<ExportLink />
<br />
<Button appearance="primary" onClick={onClickCopyToClipboard}>
Copy to clipboard
</Button>
</>
);
};

export const ExportPanel = () => {
const {
dispatch,
state: { showExportPanel, themeName, brand, lightThemeOverrides, darkThemeOverrides },
} = useThemeDesigner();

const [selectedValue, setSelectedValue] = React.useState<TabValue>('Code');

const onCloseExportPanel = () => {
dispatch({ type: 'showExportPanel', payload: false });
};

const styles = useStyles();

const [selectedValue, setSelectedValue] = React.useState<TabValue>('Code');

const onTabSelect = (event: SelectTabEvent, data: SelectTabData) => {
setSelectedValue(data.value);
};

const styles = useStyles();

const codeValue = dedent`
const ${themeName}: BrandVariants = { ${objectToString(brand, '\u00A0\u00A0')} };

Expand All @@ -75,24 +134,31 @@ export const ExportPanel = () => {
const darkTheme: Theme = {
...createDarkTheme(${themeName}), ${getBrandValues(brand, darkThemeOverrides, themeName, '\u00A0\u00A0')} };


darkTheme.colorBrandForeground1 = ${themeName}[110];
darkTheme.colorBrandForeground2 = ${themeName}[120];
`;

const jsonLightValue = dedent`
${JSON.stringify({ ...createLightTheme(brand), ...lightThemeOverrides }, null, '\t')}`;
${JSON.stringify(
{
...createLightTheme(brand),
...lightThemeOverrides
},
null,
'\t'
)}
`;

const jsonDarkValue = dedent`
${JSON.stringify(
{
...createDarkTheme(brand),
...{ colorBrandForeground1: brand[110], colorBrandForeground2: brand[120] },
...darkThemeOverrides,
},
null,
'\t',
)}
${JSON.stringify(
{
...createDarkTheme(brand),
...{ colorBrandForeground1: brand[110], colorBrandForeground2: brand[120] },
...darkThemeOverrides,
},
null,
'\t',
)}
`;

const exportedValue = React.useMemo(() => {
Expand All @@ -116,67 +182,15 @@ export const ExportPanel = () => {
<>
{showExportPanel && (
<FluentProvider theme={webLightTheme}>
<div
style={{
zIndex: 100,
position: 'absolute',
top: '0px',
right: '0px',
width: '400px',
border: `1px solid ${tokens.colorNeutralStroke1}`,
borderRadius: tokens.borderRadiusXLarge,
backgroundColor: tokens.colorNeutralBackground1,
boxShadow: tokens.shadow64,
}}
>
<div style={{ margin: '16px' }}>
<div className={styles.exportHeader}>
<Text as="h1" id="headingID" size={500}>
Export Theme
</Text>
<Button
size="small"
appearance="subtle"
icon={<DismissSquare24Regular />}
// eslint-disable-next-line react/jsx-no-bind
onClick={onCloseExportPanel}
/>
</div>

<br />
<Body1>
Passing this theme to a FluentProvider will automatically apply it to any Fluent components below it.
You can also export this to CodeSandbox with a few component examples below.
</Body1>
<br />
<TabList
defaultSelectedValue="Code"
selectedValue={selectedValue}
onTabSelect={onTabSelect} // eslint-disable-line react/jsx-no-bind
>
<Tab value="Code">Code</Tab>
<Tab value="JSONLight">JSON (light)</Tab>
<Tab value="JSONDark">JSON (dark)</Tab>
</TabList>
<Textarea
className={styles.text}
size="small"
value={exportedValue}
id={'textArea'}
textarea={{ className: styles.textarea }}
readOnly
/>
<br />
<ExportLink />
<br />
<Button
appearance="primary"
// eslint-disable-next-line react/jsx-no-bind
onClick={onClickCopyToClipboard}
>
Copy to clipboard
</Button>
</div>
<div className={styles.panelContainer}>
<ExportPanelHeader onClose={onCloseExportPanel} />
Copy link

@mltejera mltejera Feb 18, 2025

Choose a reason for hiding this comment

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

I see you also split things out into separate components here. While I think the intention is noble, it may be making things harder to reason about for a component this size.

<br />
Copy link

@mltejera mltejera Feb 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I know this is not your code, but we should remove random line breaks.

They're often improperly used by people unfamiliar with CSS layout techniques to give margin or spacing.

They really should only be used in long form, paragraph text content.

<ExportPanelContent
selectedValue={selectedValue}
onTabSelect={onTabSelect}
exportedValue={exportedValue}
onClickCopyToClipboard={onClickCopyToClipboard}
/>
</div>
</FluentProvider>
)}
Expand Down
Loading