Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat(Loader): add delay prop #969

Merged
merged 5 commits into from
Feb 27, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import * as React from 'react'
import Knobs from 'docs/src/components/Knobs/Knobs'

type LoaderExampleLoaderKnobsProps = {
mounted?: boolean
onKnobChange: () => void
}

const LoaderExampleLoaderKnobs: React.FC<LoaderExampleLoaderKnobsProps> = props => {
const { mounted, onKnobChange } = props

return (
<Knobs>
<Knobs.Boolean name="mounted" onChange={onKnobChange} value={mounted} />
</Knobs>
)
}

LoaderExampleLoaderKnobs.defaultProps = {
mounted: true,
}

export default LoaderExampleLoaderKnobs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { Loader } from '@stardust-ui/react'
import * as React from 'react'

const LoaderExampleDelay: React.FC<{ knobs: { mounted: boolean } }> = ({ knobs }) => (
<div style={{ minHeight: 24 }}>{knobs.mounted && <Loader delay={500} />}</div>
Copy link
Contributor

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?

Copy link
Member Author

@layershifter layershifter Feb 27, 2019

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to avoid this effect:

loader-delay

Copy link
Contributor

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..

)

export default LoaderExampleDelay
16 changes: 16 additions & 0 deletions docs/src/examples/components/Loader/Usage/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import * as React from 'react'

import ComponentExample from 'docs/src/components/ComponentDoc/ComponentExample'
import ExampleSection from 'docs/src/components/ComponentDoc/ExampleSection'

const LoaderUsageExamples = () => (
<ExampleSection title="Usage">
<ComponentExample
title="Delay"
description="Time in milliseconds after component mount before spinner is visible."
examplePath="components/Loader/Usage/LoaderExampleDelay"
/>
</ExampleSection>
)

export default LoaderUsageExamples
2 changes: 2 additions & 0 deletions docs/src/examples/components/Loader/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@ import * as React from 'react'

import Performance from './Performance'
import Types from './Types'
import Usage from './Usage'
import Variations from './Variations'

const LoaderExamples = () => (
<>
<Types />
<Variations />
<Usage />
<Performance />
</>
)
Expand Down
54 changes: 49 additions & 5 deletions packages/react/src/components/Loader/Loader.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,15 @@ export interface LoaderProps extends UIComponentProps, ColorComponentProps {
*/
accessibility?: Accessibility

/** Time in milliseconds after component mount before spinner is visible. */
delay?: number

/** A loader can contain an indicator. */
indicator?: ShorthandValue

/** Loaders can appear inline with content. */
inline?: boolean
Copy link
Member Author

Choose a reason for hiding this comment

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

Added missing prop definition


/** A loader can contain a label. */
label?: ShorthandValue

Expand All @@ -37,10 +43,14 @@ export interface LoaderProps extends UIComponentProps, ColorComponentProps {
size?: SizeValue
}

export interface LoaderState {
visible: boolean
}

/**
* A Loader indicates a possible user action.
*/
class Loader extends UIComponent<ReactProps<LoaderProps>> {
class Loader extends UIComponent<ReactProps<LoaderProps>, LoaderState> {
static create: Function
static displayName = 'Loader'
static className = 'ui-loader'
Expand All @@ -51,27 +61,61 @@ class Loader extends UIComponent<ReactProps<LoaderProps>> {
content: false,
color: true,
}),
delay: PropTypes.number,
indicator: customPropTypes.itemShorthand,
inline: PropTypes.bool,
label: customPropTypes.itemShorthand,
labelPosition: PropTypes.oneOf(['above', 'below', 'start', 'end']),
size: customPropTypes.size,
}

static defaultProps = {
accessibility: loaderBehavior,
delay: 0,
Copy link
Contributor

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?

Copy link
Member Author

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.

indicator: '',
labelPosition: 'below',
size: 'medium',
}

delayTimer: number

constructor(props, context) {
super(props, context)

this.state = {
visible: this.props.delay === 0,
}
}

componentDidMount() {
const { delay } = this.props

if (delay > 0) {
this.delayTimer = window.setTimeout(() => {
this.setState({ visible: true })
Copy link
Contributor

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

Copy link
Member Author

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.

}, delay)
}
}

componentWillUnmount() {
clearTimeout(this.delayTimer)
Copy link
Contributor

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

Copy link
Member Author

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

image

Chrome

image

}

renderComponent({ ElementType, classes, accessibility, variables, styles, unhandledProps }) {
const { indicator, label } = this.props
const { visible } = this.state

return (
<ElementType className={classes.root} {...accessibility.attributes.root} {...unhandledProps}>
{Box.create(indicator, { defaultProps: { styles: styles.indicator } })}
{Box.create(label, { defaultProps: { styles: styles.label } })}
</ElementType>
visible && (
<ElementType
className={classes.root}
{...accessibility.attributes.root}
{...unhandledProps}
>
{Box.create(indicator, { defaultProps: { styles: styles.indicator } })}
{Box.create(label, { defaultProps: { styles: styles.label } })}
</ElementType>
)
)
}
}
Expand Down