Skip to content
This repository has been archived by the owner on Dec 13, 2018. It is now read-only.

Improve changing the underlying component that's rendered #135

Closed
kentcdodds opened this issue May 25, 2017 · 8 comments · Fixed by #168
Closed

Improve changing the underlying component that's rendered #135

kentcdodds opened this issue May 25, 2017 · 8 comments · Fixed by #168

Comments

@kentcdodds
Copy link
Contributor

In the v2 styled-components announcement blogpost, they introduced a new API called the .extend helper. We actually do not need this because our glamorous(Component) already behaves in this way. So that's a relief (however we may be able to improve docs so this is more clear 🤔).

But they also introduced a .withComponent API which I think is kinda handy:

const Heading = styled.h1`
  /* ... */
`
export const H1 = Heading
export const H2 = Heading.withComponent('h2')
export const H3 = Heading.withComponent('h3')
export const LinkHeading = Heading.withComponent(Link)

For us, to do this now is not really ergonomic:

const Heading = glamorous.h1({})

const H2 = glamorous.h2(Heading.styles)

Should we add a withComponent API?

const Heading = glamorous.h1({})
const H2 = Heading.withComponent('h2')

Please leave a 👍 (yes) or 👎 (no, don't add the new API)

@kentcdodds
Copy link
Contributor Author

Ok, so I created this as a proof of concept way to do this without building something into glamorous. However, it's a bit of a hack and what's worse is it results in a full render of the original component which could have unintended side effects.

So I think that we'll want this built-into glamorous 👍 Anyone wanna work on this? Here's where we render the component. We'll basically want to add to this Object.assign call and we'll want that to add a function called withComponent which accepts a component and assigns the value of GlamorousComponent.comp and even GlamorousComponent.rootEl I think. I'm thinking that the withComponent API should be similar to the glamorous function API: withComponent(comp, {rootEl = comp.comp ? comp.comp : comp, displayName = glamorous(${getDisplayName(comp)}), forwardProps = []} = {}). Then overriding those properties on GlamorousComponents.

I'm pretty sure that's all that needs to happen. We'll want to have a handful of tests for this functionality. I would be cool if we make a brand new file for these tests: __tests__/create-glamorous.withComponent.js to make things more manageable.

Who wants this one?

@ansumanshah
Copy link

@kentcdodds would love to try

@ansumanshah
Copy link

Unable to get this working, how will I change values of GlamorousComponent inside the withComponent function. Maybe I am doing something terribly wrong or have no clue what's happening.

@kentcdodds
Copy link
Contributor Author

I would create a new function declaration here. Then on the line after this one I would add another object parameter to the Object.assign call: {withComponent}

So it'll look like:

// ...
      Object.assign(
        GlamorousComponent,
        getGlamorousComponentMetadata({
          comp,
          styles,
          rootEl,
          forwardProps,
          displayName,
        }),
        {withComponent},
      )
      return GlamorousComponent
    }
  }

  function withComponent(
    comp,
    {
      rootEl = comp.comp ? comp.comp : comp,
      displayName = `glamorous(${getDisplayName(comp)})`,
      forwardProps = GlamorousComponent.forwardProps
    } = {}
  ) {
    Object.assign(GlamorousComponent, {comp, rootEl, displayName, forwardProps})
  }

  function getGlamorousComponentMetadata({
  // ...

I'm pretty sure that's actually all that you'd need to do. Could be more to it... But from there it's just writing a few tests 😄

Let me know if you need anymore help!

@ansumanshah
Copy link

did nearly the same thing but GlamorousComponent is not defined in scope

@kentcdodds
Copy link
Contributor Author

Ah, put it up a couple lines so it's in the right closure :)

@ansumanshah
Copy link

ansumanshah commented Jun 1, 2017

Moving it up a scope does not solve the issue. It changes the tag of the actual component too

@kentcdodds
Copy link
Contributor Author

Why don't you go ahead and push what you have in a PR and we'll give it a look.

kentcdodds pushed a commit that referenced this issue Jun 8, 2017
kentcdodds pushed a commit that referenced this issue Jun 8, 2017
* feat(glamororus): withComponent support

Closes #135

* test(withComponent): add more test for with-commponent
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants