-
Notifications
You must be signed in to change notification settings - Fork 55
Conversation
/** A loader can contain an indicator. */ | ||
indicator?: ShorthandValue | ||
|
||
/** Loaders can appear inline with content. */ | ||
inline?: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be provide a more reasonable default? Maybe 100ms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we should clear the timeout here too and set this.delayTimer = null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check if this.delayTimer is not null/undefined before calling clearTimeout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use here Flex with padding instead of div?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).