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

[Skeleton] Add Joy UI Skeleton component #37893

Merged
merged 35 commits into from
Jul 18, 2023
Merged

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jul 10, 2023

@mui-bot
Copy link

mui-bot commented Jul 10, 2023

Netlify deploy preview

@mui/joy: parsed: +1.43% , gzip: +1.42%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 5093cd7

@siriwatknp siriwatknp changed the title Joy/skeleton [Skeleton] Add Joy UI Skeleton component Jul 10, 2023
@siriwatknp siriwatknp added component: skeleton This is the name of the generic UI component, not the React module! package: joy-ui Specific to @mui/joy labels Jul 10, 2023
@zanivan
Copy link
Contributor

zanivan commented Jul 10, 2023

Great work, @siriwatknp !
One thing, though—is there a reason why the default state is set to pulse? I believe the most common out there is the wave, so I wonder if we shouldn't set it as default instead.

@siriwatknp
Copy link
Member Author

Great work, @siriwatknp ! One thing, though—is there a reason why the default state is set to pulse? I believe the most common out there is the wave, so I wonder if we shouldn't set it as default instead.

The only reason is that I follow the Skeleton from Material UI but we can use a different default animation for Joy UI. I will update the default animation to wave.

@siriwatknp siriwatknp marked this pull request as ready for review July 12, 2023 10:00
@siriwatknp siriwatknp requested a review from zanivan July 12, 2023 10:00
@siriwatknp
Copy link
Member Author

@zanivan I think I know why Material UI uses pulse as a default animation same as Chakra UI, Mantine, and Shadcn/ui

Screen.Recording.2566-07-13.at.17.22.38.mov

It's tricky to make the wave go from left to right for the entire interface. Should we switch back to pulse? @zanivan

@zanivan
Copy link
Contributor

zanivan commented Jul 13, 2023

Yep, now I see it! I think we can roll back to pulse then

@siriwatknp
Copy link
Member Author

@zanivan I think it is ready!

@siriwatknp siriwatknp added the design: joy This is about Joy Design, please involve a visual or UX designer in the process label Jul 17, 2023
Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

Looking awesome to me! 🎉 Pushed a couple of docs-related changes but would love a review from @samuelsycamore for a much better copywriting perspective 🤙

@siriwatknp siriwatknp merged commit a748b51 into mui:master Jul 18, 2023
<Skeleton variant="overlay">
<img
alt=""
src="https://images.unsplash.com/photo-1686548812883-9d3777f4c137"
Copy link
Member

Choose a reason for hiding this comment

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

This image is huge: 4 MB, fixed in 08adce6

Copy link
Member Author

Choose a reason for hiding this comment

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

Works for me.

Comment on lines +12 to +13
<img
alt=""
Copy link
Member

@oliviertassinari oliviertassinari Jul 21, 2023

Choose a reason for hiding this comment

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

I don't understand this demo. As far as I understand the use case for the skeleton, it's to wait for the API to return the data, so we don't know the image URL at this point, so why is not a Skeleton with no child?

Suggested change
<img
alt=""

If it's because the image already has its layout, then

Suggested change
<img
alt=""
<img
alt=""
src="data:image/gif;base64,R0lGODlhAQABAAD/ACwAAAAAAQABAAACADs="

would probably make more sense.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 22, 2023

It's tricky to make the wave go from left to right for the entire interface.

It might be more than this. Facebook used to do it right with "wave", they moved to "pulse". https://www.tiktok.com/ uses wave and it doesn't feel great.

I almost wonder if we shouldn't remove "wave". https://www.quora.com/answer does the "wave" well though. You can see it by blocking https://qsbr.cf2.quoracdn.net/-4-ans_frontend-relay-component-Multifeed-27-b8def12679df30e1.webpack
The way it's done is… they create one animation for the whole area, and add white background areas to hide the parts that needs to be stable.


One bug:

Screen.Recording.2023-07-22.at.02.04.03.mov

https://master--material-ui.netlify.app/joy-ui/react-skeleton/

@mapache-salvaje
Copy link
Contributor

@siriwatknp just wanted to say that although I never got a chance to review the docs for this PR, I don't think I needed to honestly. Your writing has gotten really good over the last year, and I can tell that you've internalized the company style guide at this point because I don't really have any copyediting suggestions. Great work!

@siriwatknp
Copy link
Member Author

@siriwatknp just wanted to say that although I never got a chance to review the docs for this PR, I don't think I needed to honestly. Your writing has gotten really good over the last year, and I can tell that you've internalized the company style guide at this point because I don't really have any copyediting suggestions. Great work!

That's because of your writing style guide 😚.

@PupoSDC
Copy link
Contributor

PupoSDC commented Aug 2, 2023

Uhuhuhuh Thanks a lot for this! I was caught by surprise when importing my custom skeleton and it pulled tje joy one instea!

<3 <3 <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: skeleton This is the name of the generic UI component, not the React module! design: joy This is about Joy Design, please involve a visual or UX designer in the process package: joy-ui Specific to @mui/joy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Joy] Add the Skeleton component
7 participants