✨ fallback component #85

ptitFicus · 2017-11-04T16:41:29Z

fixes #49

Hi,

I adapted code of #66 (initially made for #49) to the new version of this HOC.

I also:

Changed isInState name to hasStatus: it may still be unclear but a last it's not confusing with react state anymore
Factorized the whole coloration code in a util folder. It avoids duplication but util folder is not necessarily a very good pattern: any idea welcome 😊

The is some linting issues, since I call setState in componentDidMount directly 😱, however the old componentDidMount dit already call setState, it was just hidden in the setColor function. We should either change linting rules or find a new color strategy.

README is missing an explicative section about this improvement, but I would like to make sure we all agree on this before writting it.

Let's talk about this again! 😃

coveralls · 2017-11-04T16:43:52Z

Coverage increased (+0.3%) to 98.795% when pulling 2c4da69 on fallback into b3f2b3a on master.

fabienjuif · 2017-11-07T08:29:48Z

Take care, you don't use you SSH key to push those commits.
So your name is the not the right one on commits ;)

fabienjuif · 2017-11-07T08:32:29Z

src/ErrorCross/ErrorCross.jsx

+    return (
+      <div
+        title={message}
+        ref={(c) => { this.div = c }}


Now I prefer to do this :

class extends Component { attach: (div) => { const newColor = findCorrectColor(div) if (this.state.color !== newColor) { this.setState({ color: newColor }) } } render () { return <div ref={this.attach} /> } }

So I am sure of the order, and I don't rely on the this context

Nice pattern, I'll change it

fabienjuif · 2017-11-07T08:33:21Z

src/ErrorCross/ErrorCross.jsx

+        >
+          <path
+            stroke={color}
+            strokeWidth="3.03754568"


What a precision :)

fabienjuif · 2017-11-07T08:33:39Z

src/ErrorCross/ErrorCross.jsx

+  }
+}
+
+const { string, object } = PropTypes


clever one :p

fabienjuif · 2017-11-07T08:36:09Z

src/TailSpin/TailSpin.jsx

-      this.setState({
-        color: color.toHexString(),
-      })
+    const newColor = findCorrectColor(this.svg)


Same comment as before (attach method)

fabienjuif · 2017-11-07T08:40:10Z

src/core.jsx

-    if (print === undefined) {
-      const { loaded } = props
-      return loaded === undefined ? true : !!loaded
+  const hasStatus = (prop, propProcessor, defaultProp, defaultValue) => (props, state, context) => {


hasStatus should be before the export default, so the function is not created for each HOC.

fabienjuif · 2017-11-07T08:41:39Z

src/core.jsx


      render() {
-        if (!isLoaded(this.props, this.state, this.context)) {
+        if (isInError(this.props, this.state, this.context)) {


We will avoid negative test and else :

if (isInError) return <ErrorIndicator ... if (isLoaded) return <ComposedComponent ... return <LoadingIndicator ...

fabienjuif · 2017-11-07T08:45:32Z

src/core.jsx

-        .map(p => Boolean(props[p]))
-        .reduce((allProps, currentProp) => allProps && currentProp)
+    if (Array.isArray(prop)) {
+      const boolProps = prop.map(p => Boolean(props[p]))


We should avoir this to kind of writing (I know this was the case before, you can open an issue if you want to argue).
But here in hasStatus we have !! technique and Boolean technique

You're right, using both is confusing, I'll switch to !! here

fabienjuif · 2017-11-07T08:47:49Z

src/index.spec.js

    })
  })
+
+  describe('error parameter', () => {


Nice 👍

What do you think of this new style of writing our test here ?

That's actuelly pretty cool =)

fabienjuif · 2017-11-07T08:48:19Z

src/utils/colorUtil.js

+  return window.getComputedStyle(node, null).getPropertyValue('background-color')
+}
+
+export const initialColor = '#cecece'


Do we have to use UPPER_CASE convention for constants ?

Good idea =)

fabienjuif · 2017-11-07T08:49:18Z

I renamed your PR so the squash/merge would have a better message to me

fabienjuif

Very good one.
Thank you for the reactivity 😃

I made some comments above, feel free to open issues if you think this is out of this PR scope.
And feel free to argue ;)

fabienjuif · 2017-11-07T08:55:28Z

And I update your first post to add the fixes triggered word

coveralls · 2017-11-07T22:26:08Z

Coverage increased (+0.3%) to 98.765% when pulling 9c60ae5 on fallback into b3f2b3a on master.

fabienjuif · 2017-11-08T09:15:30Z

Nice job @ptitFicus 👍
Do you want to rebase your branch so you can set the author right ?

git rebase -i master
set e (edit) instead of pick for each commit
For each commit:
- git commit --amend --author 'Benjamin CAVY <benjamin.cavy@gmail.com>'
- git rebase --continue
then... push -f

coveralls · 2017-11-10T19:02:10Z

Coverage increased (+0.3%) to 98.765% when pulling b7840f9 on fallback into b3f2b3a on master.

ptitFicus · 2017-11-10T19:05:07Z

Thanks for the advice @fabienjuif ! I was about to give up on this author mess 😄

I added a dedicated section to the README + I updated project version.

fabienjuif · 2017-11-12T18:57:35Z

README.md

 The `print` parameter can also be a function. Then the `props` and `context` are given to it (in this order), and it should return a boolean indicating wether or not the actual component should be displayed.
+
+### Error handling
+The `error` parameter allow to specify a prop that indicates wether or not a placeholder error component should be displayed in replacement of the real component.


fabienjuif · 2017-11-12T18:58:48Z

README.md

+
+### Error handling
+The `error` parameter allow to specify a prop that indicates wether or not a placeholder error component should be displayed in replacement of the real component.
+It's usefull when data that are mandatory for the correct display of a component are missing.


I think this is @hgwood, but someone once tells me to use required instead of mandatory.
I can't remember why tho.

It's just way more common to use "required". "mandatory" is more formal. In general shorter words are preferred (eg speed vs velocity, give up vs abandon, build vs construct...). I often use the shortcut that if it's French-sounding, then it's formal :).

fabienjuif · 2017-11-12T18:59:45Z

README.md

+The `error` parameter allow to specify a prop that indicates wether or not a placeholder error component should be displayed in replacement of the real component.
+It's usefull when data that are mandatory for the correct display of a component are missing.
+
+Like for the `print` prop, `error` can be a `boolean`, a `string` (referencing a prop name), an array of `string` (an array of prop names) or a `function` (whose result will be converted to `boolean`).


As the ``print`` prop ?

Are you sure about this one ? English is not my strong suit however I think using "as" would mean that error could be use in place of print.

fabienjuif · 2017-11-12T19:00:09Z

examples/src/components/Examples/Examples.jsx

      className={styles.example}
      link="ErrorIndicator"
-      code="export&nbsp;default loader(Base)"
+      code="export&nbsp;default loader()(Base)"


fabienjuif

README.md review

fabienjuif

👌

ptitFicus requested a review from fabienjuif November 4, 2017 16:41

fabienjuif reviewed Nov 7, 2017

View reviewed changes

fabienjuif changed the title ~~✨ First draft of fallback component~~ ✨ fallback component Nov 7, 2017

fabienjuif requested changes Nov 7, 2017

View reviewed changes

ptitFicus added 5 commits November 10, 2017 20:00

✨ First draft of fallback component

93c5eed

🔨 Fix some linting

8749688

🔨 Code review improvements

a996d3c

Update README + fix example displayed code

10784bf

Update version

b7840f9

ptitFicus force-pushed the fallback branch from 9c60ae5 to b7840f9 Compare November 10, 2017 19:01

fabienjuif reviewed Nov 12, 2017

View reviewed changes

fabienjuif requested changes Nov 12, 2017

View reviewed changes

✏️ Improve README

b8d5dc2

fabienjuif approved these changes Nov 13, 2017

View reviewed changes

fabienjuif merged commit 6c86b56 into master Nov 13, 2017

fabienjuif deleted the fallback branch November 13, 2017 07:11

✨ fallback component #85

✨ fallback component #85

Uh oh!

Conversation

ptitFicus commented Nov 4, 2017 • edited by fabienjuif Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 4, 2017

Uh oh!

fabienjuif commented Nov 7, 2017

Uh oh!

fabienjuif Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabienjuif commented Nov 7, 2017

Uh oh!

fabienjuif left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabienjuif commented Nov 7, 2017

Uh oh!

coveralls commented Nov 7, 2017

Uh oh!

fabienjuif commented Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 10, 2017

Uh oh!

ptitFicus commented Nov 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabienjuif left a comment

Choose a reason for hiding this comment

Uh oh!

fabienjuif left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

ptitFicus commented Nov 4, 2017 •

edited by fabienjuif

Loading

fabienjuif Nov 7, 2017 •

edited

Loading

fabienjuif left a comment •

edited

Loading

fabienjuif commented Nov 8, 2017 •

edited

Loading