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

Conversation

gthomas-appfolio
Copy link
Contributor

No description provided.

@gthomas-appfolio
Copy link
Contributor Author

BTW I may remove the duration and segment props, just seemed harmless to add

<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>
<g transform="translate(-100,-100)">
{[...Array(segments).keys()].map(i => {
Copy link

@balloob balloob Sep 15, 2017

Choose a reason for hiding this comment

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

lol what are you doing.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha I was proud of this range hack, np will extract

Copy link

@balloob balloob left a comment

Choose a reason for hiding this comment

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

Love getting rid of more CSS 👍

@gthomas-appfolio
Copy link
Contributor Author

Actually the <animate> tag does not work on IE11... let me look further before merging

- Use CSS animaton vs <animate> tag
- Extract range
- Remove segments prop
- Move duration and size to string
- Update story
Copy link
Contributor

@tlconnor tlconnor left a comment

Choose a reason for hiding this comment

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

Looks good to me, even if you are replacing my spinner!

@gthomas-appfolio
Copy link
Contributor Author

Haha, sorry, and thanks for adding. The same SVG should be feasible to use as image in APM assets now too.

- Make classname more specific
- Remove duration from props, had animation issues on updates
<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?

@gthomas-appfolio gthomas-appfolio merged commit fc2c9fe into master Sep 16, 2017
@gthomas-appfolio gthomas-appfolio deleted the moveSpinnerToSvg branch September 16, 2017 04:43
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.

3 participants