Skip to content
This repository has been archived by the owner on Jul 3, 2023. It is now read-only.

Add loading icon component #236

Merged
merged 3 commits into from
Mar 2, 2019
Merged

Add loading icon component #236

merged 3 commits into from
Mar 2, 2019

Conversation

@BrendanAnnable BrendanAnnable temporarily deployed to nusight-pr-236 February 20, 2019 05:53 Inactive
@BrendanAnnable BrendanAnnable temporarily deployed to nusight-pr-236 March 2, 2019 01:26 Inactive
Copy link
Member

@BrendanAnnable BrendanAnnable left a comment

Choose a reason for hiding this comment

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

I had a bunch of tiny feedback items that would take a while to communicate via text, so I've just submitted a commit. It does:

  • Remove the div wrapper.
  • Simplify the styles to avoid relative/absolute positioning.
  • Set the width/height on the SVG itself.
  • Make the component a PureComponent instead of an observer (since it doesn't take any observable values)
  • Remove the classname prop, and inline the props type
  • Rename style.css to styles.css (and its local import variable)

If that LGTY then this PR LGTM 😄

@JosephusPaye
Copy link
Member Author

LGTM, thanks!

@JosephusPaye JosephusPaye merged commit c8e3d36 into master Mar 2, 2019
@BrendanAnnable BrendanAnnable deleted the paye/loading-icon branch January 15, 2020 22:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants