8000 Add Spinner story by Coteh · Pull Request #1006 · short-d/short · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Oct 17, 2020. It is now read-only.

Add Spinner story #1006

Open
wants to merge 4 commits into
Open

Add Spinner story #1006

wants to merge 4 commits into from

Conversation

Coteh
Copy link
Collaborator
@Coteh Coteh commented Aug 27, 2020 •
edited
Loading

image

@codecov
Copy link
codecov bot commented Aug 27, 2020 •
edited
Loading

Codecov Report

Merging #1006 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1006   +/-   ##
=======================================
  Coverage   52.58%   52.58%           
=======================================
  Files         142      142           
  Lines        3564     3564           
  Branches      168      168           
=======================================
  Hits         1874     1874           
  Misses       1623     1623           
  Partials       67       67
Flag Coverage Δ
#golang 72.03% <ø> (ø)
#typescript 22.40% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
frontend/src/component/ui/Spinner.tsx 0.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 394a65f...e4122f0. Read the comment docs.

@Coteh Coteh mentioned this pull request Aug 28, 2020
Open
magicoder10
magicoder10 suggested changes Aug 28, 2020
View reviewed changes
@Coteh @coderworld10
Apply suggestions from code review
1a08315 
Co-authored-by: Harry Liu <byliuyang11@gmail.com>
magicoder10
magicoder10 previously requested changes Aug 31, 2020
View reviewed changes
story/src/2-Spinner.stories.tsx Outdated
};

export const spinner = () => {
return <Spinner />;
Copy link
Member

Choose a reason for hiding this comment

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

Can we wrap the spanner with a background? It's too to see the orbitals and planet on storybook.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll see what I can do with Storybook Backgrounds. I'll wrap the component with background as fallback in case that doesn't work out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's how it looks now, using the dark color that comes with Storybook Backgrounds addon

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One caveat, if you switch background color, it'll always stay that color, even when selecting Spinner story. I'm going to fallback to wrapping.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

@Coteh Coteh dismissed magicoder10’s stale review September 4, 2020 05:20

We discussed in PR that we will let the style names go through for now, and revise the naming convention for SCSS module classnames later.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@magicoder10 magicoder10 magicoder10 left review comments

@rohithbalaji123 rohithbalaji123 Awaiting requested review from rohithbalaji123 rohithbalaji123 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@Coteh Coteh

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Coteh @magicoder10
0