Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gt - Update Spinner to use SVG vs CSS for shapes #300

Merged
merged 4 commits into from
Sep 16, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
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
68 changes: 56 additions & 12 deletions src/components/Spinner.js
Original file line number Diff line number Diff line change
@@ -1,21 +1,65 @@
import PropTypes from 'prop-types';
import React from 'react';
import styles from './Spinner.scss';
import classnames from 'classnames';

// TODO allow spinner color as a prop and/or respect the text-* classes
// TODO consider SVG for spinner
const Spinner = props => (
<div className={classnames('spinner', styles.spinner, props.className)} style={props.style}>
<div></div>
<div></div>
</div>

function range(size) {
const result = [];
for (let i = 0; i < size; i++) result.push(i);
return result;
}

// const since these don't behave well as live props, some animation issues:
const DURATION = '1s';
const SEGMENTS = 12;

const Spinner = ({ color, size, ...props }) => (
<svg
width={size}
height={size}
viewBox="-200 -200 200 200"
version="1.1"
{...props}
>
<defs>
<path id="shape" d="M20,10 A10,10 0 1 0 20,-10 L-20,-10 A10,10 0 1 0 -20,10" fill={color} />
</defs>
<style>{`
.gears-spinner {
Copy link

Choose a reason for hiding this comment

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

Let's make this a unique id stored in the state. Otherwise 2 spinners with different props will collide. (oh how much shadow DOM would have been nice here 👍 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, but note that the props do not modify the styles or animation any longer, so there's no concerns about a conflict afaik. (I removed due to them not cleanly working with animations on changes)

Copy link

Choose a reason for hiding this comment

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

In that case, shouldn't we keep them then as a CSS file that gets attached to the page like other components that inject CSS?

animation: gears-spinner-spin ${DURATION} infinite steps(${SEGMENTS});
}
@keyframes gears-spinner-spin {
100% { transform: rotate(360deg); }
}
`}</style>
<g transform="translate(-100,-100)">
<g className="gears-spinner">
{range(SEGMENTS).map(i => {
const opacity = (i / SEGMENTS).toFixed(2);
const rotate = i * (360 / SEGMENTS).toFixed(2);

return (
<use
key={i}
xlinkHref="#shape"
transform={`rotate(${rotate}) translate(70, 0)`}
opacity={opacity}
/>
);
})}
</g>
</g>
</svg>
);

Spinner.propTypes = {
className: PropTypes.string,
style: PropTypes.object,
// TODO add size prop in lieu of style
color: PropTypes.string,
size: PropTypes.string
};

Spinner.defaultProps = {
color: 'currentColor',
size: '1em'
};

export default Spinner;

103 changes: 0 additions & 103 deletions src/components/Spinner.scss

This file was deleted.

3 changes: 2 additions & 1 deletion src/components/Waiting.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export default class Waiting extends React.Component {
...props
} = this.props;

const spinnerClasses = classnames('text-white', styles.spinner);
return (
<Modal {...props} className={classnames(styles.waiting, className)} toggle={noop}>
{title ?
Expand All @@ -40,7 +41,7 @@ export default class Waiting extends React.Component {
</header>
: null}
<div className="text-center p-4">
{children || <Spinner className={styles.spinner} />}
{children || <Spinner className={spinnerClasses} />}
</div>
</Modal>
);
Expand Down
3 changes: 0 additions & 3 deletions src/components/Waiting.scss
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
@import './Spinner';

.waiting {
margin: 40vh auto; // TODO flexbox may be better for exact centering
width: 9rem;
Expand All @@ -12,6 +10,5 @@

.spinner {
font-size: 30px;
@include spinner-color(#fff);
}
}
51 changes: 27 additions & 24 deletions stories/Spinner.js
Original file line number Diff line number Diff line change
@@ -1,28 +1,31 @@
import React from 'react';
import { Spinner } from '../src';
import { Button, Spinner } from '../src';
import { storiesOf } from '@storybook/react';
import { number, select } from '@storybook/addon-knobs';

storiesOf('Spinner', module)
.addWithInfo('Default', () => (
<div>The <Spinner /> will scale with the font size</div>
))
.addWithInfo('Scaled', () => (
<div>
<Spinner style={{ fontSize: 10 }} />
<Spinner style={{ fontSize: 20 }} />
<Spinner style={{ fontSize: 40 }} />
<Spinner style={{ fontSize: 60 }} />
<Spinner style={{ fontSize: 80 }} />
<Spinner style={{ fontSize: 100 }} />
</div>
))
.addWithInfo('This is what it used to look like', () => (
<div>
<img width="10" src="https://qa.qa.appfolio.com/assets/saffron/blue_spinner-4cde7d33f280e9454273abe82e4674f90959e137f28f2c195b28fb184e9e9ead.gif" />
<img width="20" src="https://qa.qa.appfolio.com/assets/saffron/blue_spinner-4cde7d33f280e9454273abe82e4674f90959e137f28f2c195b28fb184e9e9ead.gif" />
<img width="40" src="https://qa.qa.appfolio.com/assets/saffron/blue_spinner-4cde7d33f280e9454273abe82e4674f90959e137f28f2c195b28fb184e9e9ead.gif" />
<img width="60" src="https://qa.qa.appfolio.com/assets/saffron/blue_spinner-4cde7d33f280e9454273abe82e4674f90959e137f28f2c195b28fb184e9e9ead.gif" />
<img width="80" src="https://qa.qa.appfolio.com/assets/saffron/blue_spinner-4cde7d33f280e9454273abe82e4674f90959e137f28f2c195b28fb184e9e9ead.gif" />
<img width="100" src="https://qa.qa.appfolio.com/assets/saffron/blue_spinner-4cde7d33f280e9454273abe82e4674f90959e137f28f2c195b28fb184e9e9ead.gif" />
</div>
));
.addWithInfo('Default', () => {
const color = select('color', ['text-primary', 'text-muted', 'text-info', 'text-success', 'text-warning', 'text-danger'], 'text-primary');
return (
<div>
<p style={{ fontSize: `${number('fontSize', 1, { range: true, min: 1, max: 5, step: 0.25 })}rem` }}>
The <Spinner /> will scale with the font size of its container,
</p>

<hr />
<h3>...and inherit color from it's container:</h3>
<p>
<Button color="secondary" size="lg" className="mr-3">
<Spinner /> Loading
</Button>
<Button color="primary" outline size="lg">
<Spinner /> Loading
</Button>
</p>
<h1 className={color}>
{color}: <Spinner className={color} />
</h1>
</div>
);
}
);
16 changes: 1 addition & 15 deletions test/components/Spinner.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,6 @@ import { Spinner } from '../../src';
describe('<Spinner />', () => {
it('should render correctly', () => {
const component = mount(<Spinner />);
assert.equal('<div class="spinner"><div></div><div></div></div>', component.html());
});

it('should respect the className prop', () => {
const component = mount(<Spinner className="bob" />);
assert.equal('<div class="spinner bob"><div></div><div></div></div>', component.html());
});

it('should respect the style prop', () => {
const style = { fontSize: 24 };
const component = mount(<Spinner style={style} />);
assert.equal(
'<div class="spinner" style="font-size: 24px;"><div></div><div></div></div>',
component.html()
);
assert(component);
});
});