Skip to content

feat(Spinner): Adding Spinner component - #99

Closed
danseethaler wants to merge 1 commit into
patternfly:masterfrom
danseethaler:spinner-component
Closed

danseethaler wants to merge 1 commit into
patternfly:masterfrom
danseethaler:spinner-component

Conversation

@danseethaler

Copy link
Copy Markdown
Contributor

What: Adding Spinner component from main Patternfly library.

Why: To include more components in the react implementation.

How: Adding one component file and a stories file to demo.

@danseethaler
danseethaler force-pushed the spinner-component branch 2 times, most recently from 94264ee to fe04404 Compare December 7, 2017 22:11
@priley86

priley86 commented Dec 7, 2017

Copy link
Copy Markdown
Member

thanks @danseethaler . This looks pretty reasonable.

can you deploy the storybook so @jgiardino can take a look?

I need to update the PR template...notes are in the README on this...

@priley86

priley86 commented Dec 7, 2017

Copy link
Copy Markdown
Member

Quick glance thought... I think the new convention is to put the actual implementation inside Spinner.js... with index.js just exporting that...

I will double check this for anything else I can think of first thing tomorrow. Thanks for opening this up!

@danseethaler

Copy link
Copy Markdown
Contributor Author

Well, looks like I used the storybook:deploy in a most unfortunate way. I wasn't certain which remote/branch to deploy to. Can you clarify this for me @priley86?

I ran npm run storybook:deploy -- --remote=danseethaler --branch=spinner-component but I'm guessing I should have run this in a different way since it blew away my remote branch. I'll try to recover the remote commit and reopen this PR but I may need to create another one.

@priley86

priley86 commented Dec 8, 2017

Copy link
Copy Markdown
Member

@danseethaler yes, you should use a different branch name. Apologies for any confusion. I will try to update the readme and the pull request template tomorrow.

@danseethaler

Copy link
Copy Markdown
Contributor Author

@priley86 just added #100 as a replacement to this PR.

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.

2 participants