feat(glamorous): withProps factory #149

atticoos · 2017-05-27T18:03:22Z

What: Adds withProps factory as discussed in #133

Why: A handy API for composing more than styles on a component 😃

How: glamorousComponentFactory comes with a withProps property on it. This allows you to recompose the factory with properties

const GlamorousComponent = glamorous(/* Comp */).withProps({ /* props */ })({ /* styles */ })

Note: these properties take a lower priority than properties defined on the GlamorousComponent by the user. Just like the styles.

Feedback Requested

Before finalizing documentation, I'd like to get a sense of the approach is what we'd like!

Checklist:

codecov-io · 2017-05-27T18:05:20Z

Codecov Report

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

@@          Coverage Diff          @@
##           master   #149   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          10     10           
  Lines         138    148   +10     
  Branches       35     37    +2     
=====================================
+ Hits          138    148   +10

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 9b77296...167ad34. Read the comment docs.

atticoos · 2017-05-27T18:09:14Z

Would love some 👀 from @jfrolich too!

kentcdodds

Looks really neat! Just a few comments. Would love the opinion of other maintainers. I think this is good though!

kentcdodds · 2017-05-27T18:09:13Z

src/__tests__/index.js

@@ -433,3 +433,29 @@ test('should accept user defined contextTypes', () => {
  const props = {}
  expect(dynamicStyles).toHaveBeenCalledWith(props, theme, context)
 })
+
+it('should compose a component withProps', () => {


I prefer test('composes a component...

kentcdodds · 2017-05-27T18:10:54Z

src/create-glamorous.js

+     *  props and the return value is used.
+     * @return {Function} the glamorousComponentFactory function.
+     */
+    glamorousComponentFactory.withProps = (...outerProps) => {


I prefer function declarations for all functions. Then above we can do:

glamorousComponentFactory.withProps = withProps return glamorousComponentFactory

Great point!

kentcdodds · 2017-05-27T18:11:30Z

src/create-glamorous.js

+     */
+    glamorousComponentFactory.withProps = (...outerProps) => {
+      // This is a higher order function of `glamorousComponentFactory`
+      return (...styles) => {


Let's use a function declaration here too. That way this function can have a name which will be used in stack traces 😀

atticoos · 2017-05-27T18:26:04Z

src/create-glamorous.js

+            const evaluated = typeof prop === 'function' ?
+              prop(innerProps) :
+              prop
+            return {...props, ...evaluated}


If we attempt to spread evaluated, and it evaluated as null or undefined (or any non object, really) -- that would raise an exception wouldn't it?

We should probably ensure this is an object, and perhaps test that it's a non-empty object to avoid the unnecessary operation?

The values of evaluated and props should always be objects. But we should do a falsy check here.

Agreed, but just like the dynamic styles, it sounds good to safeguard scenarios where the user may have a conditional return value, which could result in null

glamorous.input(props => { if (props.foo) { return {color: 'blue'} } return null })

glamorous.input.withProps(props => { if (props.foo) { return {type: 'number'} } return null })

That's technically valid, isn't it? Having an escape hatch where there might be an edge for the user where they might say "if the prop isn't x, then we have nothing to do here"

Kinda like

render() { const {foo, ...rest} = this.props if (foo) { return <Input {...rest} type="number" /> } return <Input {..rest} /> }

Ah, by "should always be an object", I realize you mean more that it shouldn't be an array, string, etc -- not saying that null isn't a valid return value 😃

allows composing components with predefined static or dynamic properties Reference #133

kentcdodds

Implementation looks great! Let's get some docs 👍

atticoos · 2017-05-27T19:50:24Z

I've added the docs as a subsection to glamorousComponentFactory below ...styles. Let me know if there's anything I can improve on those

kentcdodds

Just a couple of thoughts...

kentcdodds · 2017-05-27T19:48:22Z

README.md

@@ -299,6 +299,39 @@ const MyStyledDiv = glamorous.div(
 <MyStyledDiv /> // styles applied: {padding-top: 1, padding-right: 2, padding-bottom: 3, padding-left: 4} and anything coming from `extra-thing`.
 ```

+##### withProps


I just had a thought. Do you think that people may get confused and try to do:

const MyStyledInputWithProps = glamorous.input({ padding: 10 }).withProps({type: 'text'}) // or maybe: const MyStyledInput = glamorous.input({ padding: 10 }) const MyStyledInputWithProps = MyStyledInput.withProps({type: 'text'})

In interested of that, perhaps we should support both?

I think supporting both would be convenient - would your concern in #145 (comment) apply here though?

Yeah, I've been thinking about it more and I think that I'm ok with doing this after all...

It does follow familiar patterns in libraries like ImmutableJS, where you use the core API to create the subject, and you can build changes by working against the subject's API itself, and not necessarily on the core's factory API

var map = Immutable.Map({foo: 'bar'}) // factory API var map2 = map.set('bar', 'foo') // subject API

So, similarly:

var Component = glamorous.div() // factory API var Component2 = Component.withProps({foo: 'bar'}) // subject API

I do like this, as it opens the door for us to work with both in the same manner:

var Component = glamorous.div({backgroundColor: 'blue'}) var Component2 = Component.withProps({foo: 'bar'}) var Component3 = Component2.withStyle({color: 'red'})

Giving a consistent behavior to with* APIs.

I still want to think about this before committing, maybe bring in a few people to vote.

I like both, but they introduce different convention. There's some pluses and minuses I see for both. I'm going to pause on this and write up my thoughts comparing both approaches to share in the main thread of this PR later tonight or tomorrow.

kentcdodds · 2017-05-27T19:49:37Z

README.md

+// styles applied: {padding: 10}
+
+<MyStyledValidatedInput creditCard />
+// renders: <Input type="credit-card" pattern="/([0-9]{4}){4}/" />


This should render <input rather than <Input, also I'm pretty sure that credit-card is not a valid type. Which is why the pattern is good :)

Ha good catch

kentcdodds · 2017-05-30T02:11:29Z

I think we're going to opt for an integration example with recompose rather than implementing it as a built-in. Unless I'm missing something, that should be enough right?

atticoos · 2017-05-30T04:21:43Z

recompose looks fantastic. I don't see a huge benefit for incorporating this PR after seeing their library, which provides all the utility you might need to compose a HOC. Slippery slope - right? If we do withProps, where does it end? withState becomes asked for next?

I agree that the right move here is to promote the usage of recompose for situations like this, an example of recompose.withProps would be terrific.

I'm going to close this 👍

kentcdodds · 2017-07-05T19:08:42Z

Ok, with #175 done, I think we're ready to give this another go. Based on discussion in #133, I'd like to support this API:

import glamorous from 'glamorous'

const withPropsObject = {propA: 'a', propB: 'b'}

// withProps functions accept the same arguments that glamorous style functions accept.
const withPropsFunction = (props, theme, context) => ({propG: 'g', propH: 'h'})

// we'll call these withPropsThing, meaning when you see withPropsThing it could be either a function or object

const Div1 = glamorous.div({}).withProps(withPropsThing)
<Div1 /> // renders div with the props, filters out invalid `div` props like we do currently.

What do we need to do to make this happen? How can I help you @ajwhite?

atticoos · 2017-07-05T19:14:20Z

The most actionable item here is to expose theme and context to the withProps component wrapper. Happy to accept contributions to the existing work, otherwise I can likely turn something around within the next few days

kentcdodds · 2017-07-22T00:39:57Z

How can we help get this finished? I'd love to release this with the next major version

kentcdodds · 2017-07-27T02:58:13Z

I'm going to go ahead and finish this 👍

kentcdodds · 2017-07-27T05:25:24Z

This is superseded by #255

atticoos · 2017-07-27T20:56:08Z

Thanks for bringing this across the finish line, been a busy week and was on vaca!

kentcdodds · 2017-07-27T21:00:50Z

Sure thing! Thanks for all the work you've done!

atticoos force-pushed the with-props branch from 5298181 to eecd015 Compare May 27, 2017 18:04

atticoos added the enhancement label May 27, 2017

atticoos force-pushed the with-props branch 2 times, most recently from ac72282 to 8f5e395 Compare May 27, 2017 18:07

atticoos force-pushed the with-props branch from 8f5e395 to 64e1c7f Compare May 27, 2017 18:11

kentcdodds reviewed May 27, 2017

View reviewed changes

atticoos force-pushed the with-props branch 4 times, most recently from 686e740 to d1c175c Compare May 27, 2017 18:23

atticoos commented May 27, 2017

View reviewed changes

atticoos force-pushed the with-props branch from d1c175c to 6905a9f Compare May 27, 2017 18:27

feat(glamorous): withProps factory

de168f7

allows composing components with predefined static or dynamic properties Reference #133

atticoos force-pushed the with-props branch from 6905a9f to de168f7 Compare May 27, 2017 18:51

atticoos mentioned this pull request May 27, 2017

withProps helper method robinpowered/glamorous-native#47

Closed

kentcdodds reviewed May 27, 2017

View reviewed changes

atticoos force-pushed the with-props branch 3 times, most recently from c3151f8 to a978804 Compare May 27, 2017 19:50

kentcdodds reviewed May 27, 2017

View reviewed changes

docs(readme): withProps documentation

167ad34

atticoos force-pushed the with-props branch from a978804 to 167ad34 Compare May 27, 2017 20:08

atticoos closed this May 30, 2017

kentcdodds reopened this Jun 14, 2017

kentcdodds closed this Jul 27, 2017

kentcdodds deleted the with-props branch July 27, 2017 21:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(glamorous): withProps factory #149

feat(glamorous): withProps factory #149

atticoos commented May 27, 2017 •

edited

Loading

codecov-io commented May 27, 2017 •

edited

Loading

atticoos commented May 27, 2017

kentcdodds left a comment

kentcdodds May 27, 2017

kentcdodds May 27, 2017

atticoos May 27, 2017

kentcdodds May 27, 2017

atticoos May 27, 2017 •

edited

Loading

kentcdodds May 27, 2017

atticoos May 27, 2017 •

edited

Loading

atticoos May 27, 2017 •

edited

Loading

kentcdodds left a comment

atticoos commented May 27, 2017 •

edited

Loading

kentcdodds left a comment

kentcdodds May 27, 2017

atticoos May 27, 2017

kentcdodds May 27, 2017

atticoos May 27, 2017 •

edited

Loading

atticoos May 27, 2017 •

edited

Loading

kentcdodds May 27, 2017

atticoos May 27, 2017

kentcdodds commented May 30, 2017

atticoos commented May 30, 2017 •

edited

Loading

kentcdodds commented Jul 5, 2017 •

edited

Loading

atticoos commented Jul 5, 2017

kentcdodds commented Jul 22, 2017

kentcdodds commented Jul 27, 2017

kentcdodds commented Jul 27, 2017

atticoos commented Jul 27, 2017

kentcdodds commented Jul 27, 2017

feat(glamorous): withProps factory #149

feat(glamorous): withProps factory #149

Conversation

atticoos commented May 27, 2017 • edited Loading

Feedback Requested

codecov-io commented May 27, 2017 • edited Loading

Codecov Report

atticoos commented May 27, 2017

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atticoos May 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atticoos May 27, 2017 • edited Loading

Choose a reason for hiding this comment

atticoos May 27, 2017 • edited Loading

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

atticoos commented May 27, 2017 • edited Loading

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atticoos May 27, 2017 • edited Loading

Choose a reason for hiding this comment

atticoos May 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds commented May 30, 2017

atticoos commented May 30, 2017 • edited Loading

kentcdodds commented Jul 5, 2017 • edited Loading

atticoos commented Jul 5, 2017

kentcdodds commented Jul 22, 2017

kentcdodds commented Jul 27, 2017

kentcdodds commented Jul 27, 2017

atticoos commented Jul 27, 2017

kentcdodds commented Jul 27, 2017

atticoos commented May 27, 2017 •

edited

Loading

codecov-io commented May 27, 2017 •

edited

Loading

atticoos May 27, 2017 •

edited

Loading

atticoos May 27, 2017 •

edited

Loading

atticoos May 27, 2017 •

edited

Loading

atticoos commented May 27, 2017 •

edited

Loading

atticoos May 27, 2017 •

edited

Loading

atticoos May 27, 2017 •

edited

Loading

atticoos commented May 30, 2017 •

edited

Loading

kentcdodds commented Jul 5, 2017 •

edited

Loading