feat(Loader): add delay prop
#969
Conversation
layershifter
requested review from
dvdzkwsk,
kuzhelov,
levithomason,
miroslavstastny and
mnajdova
as code owners
| /** A loader can contain an indicator. */ | ||
| indicator?: ShorthandValue | ||
|
|
||
| /** Loaders can appear inline with content. */ | ||
| inline?: boolean |
There was a problem hiding this comment.
Added missing prop definition
Codecov Report
@@ Coverage Diff @@
## master #969 +/- ##
==========================================
+ Coverage 80.74% 80.77% +0.02%
==========================================
Files 665 665
Lines 8508 8521 +13
Branches 1499 1439 -60
==========================================
+ Hits 6870 6883 +13
Misses 1624 1624
Partials 14 14
Continue to review full report at Codecov.
|
| label: customPropTypes.itemShorthand, | ||
| labelPosition: PropTypes.oneOf(['above', 'below', 'start', 'end']), | ||
| size: customPropTypes.size, | ||
| } | ||
|
|
||
| static defaultProps = { | ||
| accessibility: loaderBehavior, | ||
| delay: 0, |
There was a problem hiding this comment.
Should be provide a more reasonable default? Maybe 100ms?
There was a problem hiding this comment.
By default it should 0 to keep existing behavior.
|
|
||
| if (delay > 0) { | ||
| this.delayTimer = window.setTimeout(() => { | ||
| this.setState({ visible: true }) |
There was a problem hiding this comment.
Seems like we should clear the timeout here too and set this.delayTimer = null
There was a problem hiding this comment.
They will never collide because it's called in componentDidMount(), there are no obvious reasons to clear it.
| } | ||
|
|
||
| componentWillUnmount() { | ||
| clearTimeout(this.delayTimer) |
There was a problem hiding this comment.
Check if this.delayTimer is not null/undefined before calling clearTimeout
There was a problem hiding this comment.
Can you please clarify why need it?
Passing an invalid ID to clearTimeout() silently does nothing; no exception is thrown.
https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/clearTimeout#Notes
IE 11
Chrome
|
LGTM, only thing is that maybe we should add test for this behavior? |
| import * as React from 'react' | ||
|
|
||
| const LoaderExampleDelay: React.FC<{ knobs: { mounted: boolean } }> = ({ knobs }) => ( | ||
| <div style={{ minHeight: 24 }}>{knobs.mounted && <Loader delay={500} />}</div> |
There was a problem hiding this comment.
Maybe use here Flex with padding instead of div?
There was a problem hiding this comment.
Actually, I don't think that it's a case for Flex. We require there minHeight because Loader will render null with delay, the same behavior as there: https://evergreen.segment.com/components/spinner/
Did I miss something?
There was a problem hiding this comment.
I want to avoid this effect:
There was a problem hiding this comment.
Aha, got it! Let's leave it as it is then..
…/stardust-ui/react into feat/loader-delay # Conflicts: # CHANGELOG.md
Fixes #888. See #888 (comment).