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

Introduce a Skeleton component #606

Merged
merged 3 commits into from
Apr 17, 2019
Merged

Introduce a Skeleton component #606

merged 3 commits into from
Apr 17, 2019

Conversation

willdurand
Copy link
Member

@willdurand willdurand commented Apr 17, 2019

Fixes #607
Fixes #608

Supports #379


This is a very simple Skeleton component that should fill the place where it is added. We can iterate if we need to add constraints (like range and minWidth props) but it's only the beginning of the long journey to fix #379.

2019-04-17 16 54 05

The second commit refactors the VersionChooser component to see the Skeleton in action:

2019-04-17 17 15 21

2019-04-17 17 17 09

@codecov-io
Copy link

codecov-io commented Apr 17, 2019

Codecov Report

Merging #606 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #606      +/-   ##
=========================================
+ Coverage   98.16%   98.2%   +0.04%     
=========================================
  Files          42      43       +1     
  Lines        1087    1112      +25     
  Branches      243     254      +11     
=========================================
+ Hits         1067    1092      +25     
  Misses         19      19              
  Partials        1       1
Impacted Files Coverage Δ
src/components/VersionChooser/index.tsx 100% <100%> (ø) ⬆️
src/components/VersionSelect/index.tsx 100% <100%> (ø) ⬆️
src/components/Skeleton/index.tsx 100% <100%> (ø)
src/reducers/linter.tsx 100% <0%> (ø) ⬆️

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 a684dbe...5249ab3. Read the comment docs.

Copy link
Contributor

@bobsilverberg bobsilverberg left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks @willdurand. Just one nit/question.

r+wc

src/components/VersionSelect/index.tsx Outdated Show resolved Hide resolved
@willdurand willdurand merged commit 50be032 into master Apr 17, 2019
@willdurand willdurand deleted the skeleton branch April 17, 2019 16:43
Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

Thanks for getting this started. There are higher priority items but I agree with what you mentioned that we are investing more code debt in non-skeletonized components each day.

I would say just hold off on skeletonizing all the existing components because that will take a lot of time and I'd like to make some headway on the comments work first. However, we should try to skeletonize new components after you land this 🎉

src/components/Skeleton/styles.module.scss Show resolved Hide resolved
src/components/Skeleton/styles.module.scss Show resolved Hide resolved
src/components/Skeleton/index.tsx Show resolved Hide resolved
src/components/VersionSelect/index.tsx Show resolved Hide resolved
src/components/VersionSelect/index.tsx Show resolved Hide resolved
@kumar303
Copy link
Contributor

Sorry for the late review. I think some of my comments are worth following up on.

@willdurand
Copy link
Member Author

I would say just hold off on skeletonizing all the existing components because that will take a lot of time and I'd like to make some headway on the comments work first. However, we should try to skeletonize new components after you land this 🎉

I thought I left such a comment somewhere.. I totally agree and that was my plan too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants