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.

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

Theming for widget variants #847

Closed
tomdye opened this issue Oct 18, 2019 · 1 comment
Closed

Theming for widget variants #847

tomdye opened this issue Oct 18, 2019 · 1 comment
Assignees

Comments

@tomdye
Copy link
Member

tomdye commented Oct 18, 2019

We need to develop a way to theme widget variants.

For example, it should be possible for a user to target the raised-button or the email-input directly and theme is differently to the standard button / input.

Current code (wrong)

Currently we are going some way to achieving this using classes along with this.theme, as seen here in the raised-button render function.

The problem with this approach is that classes is an additive function whereas theme is designed to replace the existing styling. This means that the raised button, when themed, will receive the theme styles for both button and raised-button.

eg. given the following code and the current approach, the raised button will have a green background and italic purple text. This is not correct as the raisedButtonCss root class should override the buttonCss root class.

/* buttonCss */
.root {
	color: red;
	background: green;
	padding: 20px;
	font-size: 20px;
}

/* raisedButtonCss */
.root {
	color: purple;
	font-style: italic;
}
export default factory(function Basic() {
	const theme = {
		'@dojo/widgets/button': buttonCss,
		'@dojo/widgets/raised-button': raisedButtonCss
	};

	return (
		<virtual>
			<Button disabled theme={theme}>Normal Buton</Button>
			<RaisedButton disabled theme={theme}>Raised Button</RaisedButton>
		</virtual>
	);
});

In order to achieve the correct result, where we only get the styles from the raisedButtonCss and see purple italics without the green background we need to do something cleverer with this.theme and the object that gets passed to the button.

Class based solution (better)

To fix this issue correctly and allow widget variants (ie. raised-button) to be fully themeable we need to compose together the themed classes for both button and raised-button and then pass this object as theme to the underlying widget.

@theme(css)
@theme(buttonCss)
export class RaisedButton extends ThemedMixin(WidgetBase)<RaisedButtonProperties> {
	protected render(): DNode {
		const classes = Object.keys(css).reduce((existing, key) => {
			(existing as any)[key] = this.theme((css as any)[key]);
			return existing;
		}, {});

		const buttonClasses = Object.keys(buttonCss).reduce((existing, key) => {
			(existing as any)[key] = this.theme((css as any)[key]);
			return existing;
		}, {});

		const combined = { ...buttonClasses, ...classes };

		return (
			<Button
				{...this.properties}
				theme={{
					'@dojo/widgets/button': combined
				}}
			>
				{this.children}
			</Button>
		);
	}
}

export default RaisedButton;

This proposed solution to be adopted by all widget variants appears to work and in the previous example, the raised-button gets only the appropriate theme styles applied to it.

Function based widget approach (best +++)

The proposed solution for this theming issue when using function based widgets and the theme middleware is the simplest approach by far.

export const RaisedButton = factory(function({
	properties,
	children,
	middleware: { theme }
}) {
	const props = properties();

	return (
		<Button {...props} theme={{
			...props.theme,
			'@dojo/widgets/button': { ...theme.classes(buttonCss), ...theme.classes(variantCss)}
		}}>
			{children()}
		</Button>
	);
});

For this reason, I believe that all widget variants should be created using the function based approach.

Downsides of this approach

It is a known issue that changing the theme object that is passed down will mean that any child widgets will receive the adjusted theme. However, as it is highly unlikely that a button will contain another button, or an input etc, I do not believe this will be an issue and cannot think of any realistic scenarios in which this would be problematic.

@agubler agubler mentioned this issue Nov 25, 2019
5 tasks
@tomdye tomdye mentioned this issue Nov 26, 2019
5 tasks
@vansimke vansimke mentioned this issue Nov 26, 2019
5 tasks
@tomdye tomdye mentioned this issue Nov 26, 2019
5 tasks
@vansimke vansimke mentioned this issue Nov 26, 2019
5 tasks
@tomdye tomdye self-assigned this Dec 2, 2019
@jellison jellison mentioned this issue Dec 2, 2019
5 tasks
@devpaul devpaul mentioned this issue Dec 2, 2019
5 tasks
@maier49 maier49 mentioned this issue Dec 3, 2019
5 tasks
@agubler agubler mentioned this issue Dec 4, 2019
6 tasks
@KaneFreeman KaneFreeman mentioned this issue Jun 4, 2021
7 tasks
@tomdye tomdye mentioned this issue Jun 25, 2021
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant