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
feat(skeleton-loader): add ability to customize #1814
Conversation
✔️ Deploy Preview for paste-docs ready! 🔨 Explore the source changes: 2a1a38f 🔍 Inspect the deploy log: https://app.netlify.com/sites/paste-docs/deploys/612ff80b39453f00074a97dd 😎 Browse the preview: https://deploy-preview-1814--paste-docs.netlify.app |
Size Change: +66 B (0%) Total Size: 634 kB
ℹ️ View Unchanged
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
2afd827
to
1ef8508
Compare
🦋 Changeset detectedLatest commit: 2a1a38f The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The story looks good, maybe it would be more clear to have them in a table or paragraph format, just to be a bit more realistic? Not necessary though.
I still think that animation should be more visible...
@@ -59,6 +62,7 @@ const SkeletonLoader = React.forwardRef<HTMLDivElement, SkeletonLoaderProps>( | |||
borderTopLeftRadius={borderTopLeftRadius} | |||
borderTopRightRadius={borderTopRightRadius} | |||
display={display} | |||
element={element} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does skeleton loader not have any proptypes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I think mainly the reason is going to be that because it just extends a bunch of Box props, validating those is really expensive.
We used to do it on Box itself, and it turned out to be such a performance hit in development for engineers we removed them. Having to look up the values of these props on the theme is pretty expensive performance wise.
Other reasoning is that the values are part of the design tokens theme, so there is a reference for you to get the right thing
@@ -54,7 +54,7 @@ export const decorators = [ | |||
default: | |||
case 'default': | |||
return ( | |||
<Theme.Provider theme={theme}> | |||
<Theme.Provider theme={theme} disableAnimations={isChromatic()}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
omg, this is smart.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, I forgot to press submit on calling this out. 🤞🏼 it actually works. I think it will...
01d14ef
to
2a1a38f
Compare
Enable skeleton loader customization via the customization provider