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

Add useComposedCssClasses hook #61

Merged
merged 8 commits into from
Dec 2, 2021
Merged

Add useComposedCssClasses hook #61

merged 8 commits into from
Dec 2, 2021

Conversation

cea2aj
Copy link
Member

@cea2aj cea2aj commented Nov 24, 2021

Add the ability to compose CSS classes

This hook allows custom CSS classes to be combined in various ways with the default styling of a given component. Each component will have a composition prop which will determine how the composition behaves. It is a wrapper around css-modules-theme. This PR doesn't integrate the functionality into any components yet since the use of Tailwind is still up in the air, and that would affect the integration. However once that's decided, this utility will be added to all components.

The function seemed to impact performance a bit, but adding useMemo fixes that

J=1639
TEST=manual

Test this functionality in the Searchbar with Tailwind and test that the component's built in styling works as well as custom styling.

@coveralls
Copy link

coveralls commented Nov 24, 2021

Coverage Status

Coverage decreased (-6.1%) to 69.388% when pulling 3f01557 on dev/add-injectable-css into 0612924 on main.

sample-app/src/utils/composeCssClasses.tsx Outdated Show resolved Hide resolved
* @param composition The method of combining the built-in classes with the custom classes
* @returns The composed CSS classes
*/
export function composeCssClasses<ClassInterface> (
Copy link
Contributor

Choose a reason for hiding this comment

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

what is ClassInterface?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just a generic. I can switch to T since I think that would make it clearer

Copy link
Contributor

Choose a reason for hiding this comment

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

Should the generic extend some interface or does it really let you pass anything?

Copy link
Contributor

@oshi97 oshi97 Nov 29, 2021

Choose a reason for hiding this comment

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

oh I'm also cool calling it ClassInterface instead of just T which imo is not as descriptive! My brain just derped out and didn't recognize it as a generic

Copy link
Member Author

Choose a reason for hiding this comment

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

The ClassInterface should be an object with only strings as keys and strings as values (essentially a Record<string,string>). However using that Record causes an issue for Typescript because interfaces with a specific set of keys cannot be indexed by arbitrary strings. Each class interface will have an interface with a specific set of keys, and that's not compatible with Record<string,string>. I created the isThemeObject method so that I could confirm that the builtInClasses and the customClasses objects can be safely indexed by any string. The Theme object from css-modules-theme is simply interface Theme { [key: string]: string; }.

@cea2aj cea2aj changed the title Add composeCssClasses functionality Add useComposedCssClasses hook Nov 30, 2021
@cea2aj cea2aj merged commit 54c078d into main Dec 2, 2021
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

Successfully merging this pull request may close these issues.

None yet

4 participants