Skip to content

Commit

Permalink
update apply theme to make more generic
Browse files Browse the repository at this point in the history
  • Loading branch information
@matt-hardman
matt-hardman committed Oct 16, 2020
1 parent 09cb824 commit 21255da
Showing 1 changed file with 8 additions and 5 deletions.
13 changes: 8 additions & 5 deletions src/Theme/index.tsx
View file
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
import React, { HTMLAttributes } from "react";
import cx from "classnames";

interface Options extends HTMLAttributes<HTMLElement> {
className?: string;
}
interface Options extends HTMLAttributes<HTMLElement> {}

export const applyTheme = <T extends Options>(
Component: React.FC<T>,
{ className }: Options
options: T
) => (props: T) => {
return <Component {...props} className={cx([className, props.className])} />;
return (
<Component
{...props}
className={cx([options.className, props.className])}

This comment has been minimized.

Sign in to view

Copy link
@S-unya

S-unya Oct 16, 2020

Member

Hi @matt-hardman , we are hoping to enshrine certain practices in the construction of Components, and one of them is that parent Components should feel free to pass styles into children. That is the purpose of destructuring out className as a named input (it is part of the HTMLAttributes interface, so safe to do).

The other question here is about the use of the HTMLElement generic passed to the HTMLAttributes interface. Idealy people would pass the specific HTML element so that they could get the methods for that element - e.g. form inputs have onChange, etc

Otherwise, I like the idea of passing in a generic to make this more typeable.

This comment has been minimized.

Sign in to view

Copy link
@matt-hardman

matt-hardman Oct 20, 2020 •
edited
Loading

Author Contributor

Hi Sunya, I just changed the named import to allow me to pass in additional props when applying the theme to the component, it should actually be:

<Component {...props} {...options} className={cx([options.className, props.className])} />```

When i'm using the component it then infers the types from the Component i'm passing in.

I'll update it because i missed off the `{...options}`.

Would you rather i destructured the className separately and then spread the remainder though (is that what you mean??):
i.e. ```Component: React.FC<T>,
{ className, ...rest }: T```
?

So the update would look like:
```tsx
export const applyTheme = <T extends Options>(
  Component: React.FC<T>,
  { className, ...options }: T
) => (props: T) => {
  return (
    <Component
      {...props}
      {...options}
      className={cx([className, props.className])}
    />
  );
};```

This comment has been minimized.

Sign in to view

Copy link
@S-unya

S-unya Oct 20, 2020

Member

I'm not sure what the difference between props.className and options.className are, but I can't see the point of them being separate unless you are hoping to pass in classNames from multiple components? If not, then the destructured out className is my preference - as documentation

This comment has been minimized.

Sign in to view

Copy link
@matt-hardman

matt-hardman Oct 20, 2020 •
edited
Loading

Author Contributor

Well the props and options get set at different points, it's just so i can create a create custom versions of components.

So for example I have ModalForm component where I have something like:

export const ModalFormControls = applyTheme(
    FormControls, 
    { 
        className: styles.whatever, 
        loadingIcon: <CustomLoadingIcon />
    });

and then I can use that <ModalFormControls /> component with preset styling and loading icon and then passs further classNames or props in if needed in another component.

This comment has been minimized.

Sign in to view

Copy link
@S-unya

S-unya Oct 20, 2020

Member

So, you're are adding styles from multiple components? In which case, the way you have done it makes sense, but is complex too. I think it is fine though

This comment has been minimized.

Sign in to view

Copy link
@matt-hardman

matt-hardman Oct 21, 2020

Author Contributor

Yes and other potentially other props. Ok cool thanks, as long it makes sense 👍, seems to be working ok in radioplayer.
/>
);
};

export default applyTheme;

0 comments on commit 21255da

Please sign in to comment.