Skip to content

Conversation

@bpetetot
Copy link
Collaborator

@bpetetot bpetetot commented Jan 5, 2018

fixes #67

TODO:

  • Finish tests
  • Documentation

src/core.jsx Outdated
componentDidMount() {
// set delay
if (delay) {
setTimeout(() => this.setState(state => ({ ...state, pastDelay: true })), delay)
Copy link
Member

Choose a reason for hiding this comment

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

you should register the timer and clear it in componentWillUnmount

this.timer = setTimeout()...

src/core.jsx Outdated

state = {
props: {},
pastDelay: false,
Copy link
Member

Choose a reason for hiding this comment

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

  • use print field instead of pastDelay
  • init it to true
  • in the render method you can simplify the condition : if (!this.state.print) return null

Copy link
Member

@fabienjuif fabienjuif left a comment

Choose a reason for hiding this comment

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

Some comments

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 98.864% when pulling b8dbf2b on 67-delay-parameter into 6c86b56 on master.

@bpetetot
Copy link
Collaborator Author

bpetetot commented Jan 6, 2018

@fabienjuif it's done you can check it

@bpetetot bpetetot removed the not-ready label Jan 6, 2018
@bpetetot
Copy link
Collaborator Author

bpetetot commented Jan 6, 2018

I should add some documentation in the readme file

@fabienjuif
Copy link
Member

The code is fine, I'm waiting for the documentation :)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 98.864% when pulling 72260e6 on 67-delay-parameter into 6c86b56 on master.

@ptitFicus
Copy link
Collaborator

You should probably reference the issue as well =)

README.md Outdated

When a component loads very quickly, you will see a flash of the loading component.
To avoid this behaviour, you can add a `delay` parameter to the loader with a time in milliseconds.
Then, the loading indicator will be rendered, only after this delay (of course, if the component is loaded before, then it will be rendered)
Copy link
Member

Choose a reason for hiding this comment

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

To much ponctuation for me.
Something like that maybe ?
Then, the loading indicator will be rendered after the delay if the Component can't be rendered before that.

README.md Outdated
export default loader({ print: ['data'], delay: 200 })(MyComponent)
```

By default, the no delay is defined.
Copy link
Member

Choose a reason for hiding this comment

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

By default, no delay is defined. ?

@fabienjuif
Copy link
Member

from @ptitFicus comment, I added the reference to the issue

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 98.864% when pulling ca12027 on 67-delay-parameter into 6c86b56 on master.

Copy link
Member

@fabienjuif fabienjuif left a comment

Choose a reason for hiding this comment

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

👌

@bpetetot bpetetot merged commit 1531369 into master Jan 17, 2018
@bpetetot bpetetot deleted the 67-delay-parameter branch January 17, 2018 13:52
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.

Delay parameter

5 participants