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

util: polymorphic primatives #1690

Merged
merged 11 commits into from
Mar 29, 2021
Merged

util: polymorphic primatives #1690

merged 11 commits into from
Mar 29, 2021

Conversation

danethurber
Copy link
Contributor

@danethurber danethurber commented Mar 25, 2021

new util methods to support typed(typescript) forwardRef components that support static properties, polymorphism and memoization.

for an example of how the methods would be used please see #1675 which shows refactoring the Button component and related components. Button is by far the most complicated and widely used component that supports the old polymorphic style.

@danethurber danethurber marked this pull request as ready for review March 25, 2021 17:58
: FunctionComponentWithAs<DefaultComponentType, ComponentProps>
}

export interface ForwardRefWithAsRenderFunction<
Copy link
Contributor Author

@danethurber danethurber Mar 25, 2021

Choose a reason for hiding this comment

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

oops. forgot to document using a render prop function. i'll add an example - the approach is pretty straightforward

interface Props {
  children?: React.ReactNode | (() => JSX.Element);
}
const TestInput = forwardRefWithAs<Props, "input">((props, ref) => {
  const { as: Comp = "input", children, ...rest } = props;

  return (
    <Comp ref={ref} {...rest}>
      {isFunction(children) ? children() : children}
    </Comp>
  );
});

@jhewlett
Copy link
Contributor

@danethurber So the idea here is that by asking the consumer to specify the underlying DOM element to be used, you're able to provide better autocomplete and static guarantees? (e.g. if they specify as="button", then href wouldn't be accepted as a valid prop?)

Copy link
Contributor

@KaidenR KaidenR left a comment

Choose a reason for hiding this comment

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

This looks like a really great pattern that should help a LOT with customization.

Is the idea that a component could have multiple <subcomponent>As props so that I can override different parts of the component? Like when rendering a Dropdown component I could have buttonAs and a menuAs props to customize the rendering of either of them.

(Also submitting fixes for a few typos)

docs/src/content/patterns/polymorphic.mdx Outdated Show resolved Hide resolved
docs/src/content/patterns/polymorphic.mdx Outdated Show resolved Hide resolved
docs/src/content/patterns/polymorphic.mdx Outdated Show resolved Hide resolved
docs/src/content/patterns/polymorphic.mdx Outdated Show resolved Hide resolved
danethurber and others added 4 commits March 25, 2021 13:05
Co-authored-by: Kaiden Rawlinson <kaiden-rawlinson@pluralsight.com>
Co-authored-by: Kaiden Rawlinson <kaiden-rawlinson@pluralsight.com>
Co-authored-by: Kaiden Rawlinson <kaiden-rawlinson@pluralsight.com>
Co-authored-by: Kaiden Rawlinson <kaiden-rawlinson@pluralsight.com>
@danethurber
Copy link
Contributor Author

danethurber commented Mar 25, 2021

@justin yes the types should be able to tell which attrs/props are valid for the given tag name. the component's tag name also needs to have a default value. so for example the following component defaults to a button but can be changed as needed.

const Button = forwardRefWithAs<Props, 'button'>(() => ...)

will be typed to only allow valid button props, including using HTMLButtonElement for the ref and event types. but if you used as like so

<Button as="a" />

the typings would now related to a HTMLAnchorElement so attributes like href would be valid


@KaidenR i don't have a standard answer for how we get to the internal elements yet - but your approach could definitely be something we finalize on. we might end up with a collection of compound components meant to be hyper flexible and each should allow using the as prop when needed; and we will most likely have a set of components that are convenience wrappers to provide a simpler interface that encapsulate common patterns. Some current examples being our Field components vs a convenience wrapper like the MultiSelect component.

With the addition of these typings, our compounds component would allow changing the underlying dom elements at whatever level you need. an example being.

const Button = forwardRefWithAs<Props, 'button'>(() => ...)
interface Statics {
  Button: typeof Button
}
const Compound = forwardRefWithAsAndStatics<unknown, 'div', Statics>(() ...)
Compound.Button = Button

render(
  <Compound as="section">
    <Button  />
    <Button as="a" />
  </Compound>
)

@EdwardIrby EdwardIrby merged commit 6486c6c into master Mar 29, 2021
@EdwardIrby EdwardIrby deleted the feat/primatives branch March 29, 2021 21:34
jaketrent added a commit that referenced this pull request Apr 7, 2021
* master:
  build: publish
  fix(datepicker): closes on click outside resolves #1655 (#1700)
  tagsinput: fix scroll bar position when items/pills overflow  (#1702)
  flexibility: remove base from color scale & add parseToRgb (#1696)
  fix(searchinput): weird type bug resolved (#1698)
  feat(core): temp core util for refactoring to vanillia css (#1689)
  build: publish
  select: stop intercepting all key events (#1699)
  build: publish
  fix(storybook-addon-theme): update incorrect package name in README
  feat(storybook-addon-theme): update icon to circle
  feat(storybook-addon-theme): ability to set theme per-story
  util: polymorphic primatives (#1690)
  build: publish
  fix(select): dropdown zindex added (#1691)
  Update @pluralsight/ps-design-system-storybook-addon-theme package.json for addon-catalog
  Update @pluralsight/ps-design-system-storybook-addon-center package.json for addon-catalog
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