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

feat(withComponent): add withComponent api #168

Merged
merged 2 commits into from
Jun 8, 2017
Merged

Conversation

kentcdodds
Copy link
Contributor

@kentcdodds kentcdodds commented Jun 8, 2017

Copied from #159 from @ansumanshah

What: Adds withComponent function as discussed in #135

Why: for changing the underlying component that's rendered :)

How:

function withComponent(newComp, options) {
  return glamorous(newComp, {
    ...options,
    forwardProps: GlamorousComponent.forwardProps,
  })(GlamorousComponent.styles)
}

Checklist:

  • Documentation
  • Tests
  • Code complete
  • Added myself to contributors table
  • Followed the commit message format

Closes #135

@codecov-io
Copy link

codecov-io commented Jun 8, 2017

Codecov Report

Merging #168 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #168   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          10     10           
  Lines         138    139    +1     
  Branches       35     35           
=====================================
+ Hits          138    139    +1
Impacted Files Coverage Δ
src/create-glamorous.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 964bd6c...d9ae37e. Read the comment docs.

@kentcdodds kentcdodds requested a review from atticoos June 8, 2017 17:41
function withComponent(newComp, options) {
return glamorous(newComp, {
forwardProps: GlamorousComponent.forwardProps,
...options,
Copy link
Collaborator

Choose a reason for hiding this comment

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

For some reason I always thought this would throw an exception when undefined, but I see by your unit tests, that's not the case!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, I would expect that to throw too 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

screen shot 2017-06-08 at 2 13 04 pm

Whelp, apparently it doesn't. Makes sense I suppose:

screen shot 2017-06-08 at 2 14 01 pm

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh you know.. if you were to pass it as a rest parameter, i think that's when it might:

var obj = undefined
method(...obj)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me. I defaulted it anyway just to illustrate intent (that it can be optionally provided) 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

That was interesting -- summarized: https://twitter.com/atticoos/status/872881844563447812

expect(View.displayName).toBe('View')
})

test('4esulting component can have its styles extended further', () => {

Choose a reason for hiding this comment

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

Tiny typo :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DUDE! What are you doing here!? Good to have you around 🎉

Choose a reason for hiding this comment

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

lol totally random. Thinking about converting a project to glamorous and was just poking around. 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice. Well you're always welcome here :) Let us know how we can help you!

expect(View.displayName).toBe('View')
})

test('4esulting component can have its styles extended further', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo, unless we're moving tests to 1337 speak

@@ -606,6 +606,37 @@ const MyStyledComponent = glamorous(MyComponent, {
// be forwarded to `MyComponent` because it is a valid prop for a `div`.
```

#### withComponent
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we be seeing a diff for doctoc in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we only go three levels deep for doctoc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, forgot we have a max depth!

Copy link
Collaborator

@atticoos atticoos left a comment

Choose a reason for hiding this comment

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

Nice! Things look great.

@kentcdodds kentcdodds merged commit f014fd1 into master Jun 8, 2017
@kentcdodds kentcdodds deleted the pr/with-component branch June 8, 2017 18:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve changing the underlying component that's rendered
5 participants