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

Feat(web): Introduce Skeleton component #DS-1625 #1863

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

katerinarehorkova
Copy link

Description

  • New component Skeleton
  • only web package implementation

Additional context

Issue reference

https://jira.almacareer.tech/browse/DS-1625

@katerinarehorkova katerinarehorkova added the feature New feature or request label Jan 22, 2025
Copy link

netlify bot commented Jan 22, 2025

Deploy Preview for spirit-design-system-storybook canceled.

Name Link
🔨 Latest commit 3dc28c0
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system-storybook/deploys/67a0c1606363b20008fd3a8b

Copy link

netlify bot commented Jan 22, 2025

Deploy Preview for spirit-design-system ready!

Name Link
🔨 Latest commit 3dc28c0
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system/deploys/67a0c160e092970008c87568
😎 Deploy Preview https://deploy-preview-1863--spirit-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 96 (no change from production)
Accessibility: 91 (no change from production)
Best Practices: 100 (no change from production)
SEO: 91 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@literat
Copy link
Collaborator

literat commented Jan 22, 2025

👏 Great, nice first PR and contribution to the Spirit Design System 🎉

When I switch the demo to the "Light on brand" theme, the animation and color are missing. I think that this should be the same as the default. But not sure. @crishpeen

Screenshot 2025-01-22 at 12 51 36

@katerinarehorkova katerinarehorkova marked this pull request as ready for review January 24, 2025 12:17
Copy link
Collaborator

@literat literat left a comment

Choose a reason for hiding this comment

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

Good Job. LGTM 👍 Please wait for the merge after approval by @crishpeen or @adamkudrna. Thanks.

Copy link
Member

@crishpeen crishpeen left a comment

Choose a reason for hiding this comment

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

Thank you for your work, I am sorry I have a lot of comments, especially because a lot of them is something which might be opinionated.

Sometimes I just raised a question where I am not sure about something.

Please also add a visual screenshot when the PR is ready again (there will actually be two - new web HP and the acutal Skeleton). Thank you very much and feel free to ask for more explanation if I am not clear somewhere!

packages/web/src/scss/components/Skeleton/_Skeleton.scss Outdated Show resolved Hide resolved
packages/web/src/scss/components/Skeleton/_Skeleton.scss Outdated Show resolved Hide resolved
packages/web/src/scss/components/Skeleton/_Skeleton.scss Outdated Show resolved Hide resolved
packages/web/src/scss/components/Skeleton/_Skeleton.scss Outdated Show resolved Hide resolved
packages/web/src/scss/components/Skeleton/_Skeleton.scss Outdated Show resolved Hide resolved
packages/web/src/scss/components/Skeleton/_tools.scss Outdated Show resolved Hide resolved
packages/web/src/scss/components/Skeleton/index.html Outdated Show resolved Hide resolved
packages/web/src/scss/components/Skeleton/index.html Outdated Show resolved Hide resolved
packages/web/src/scss/components/Skeleton/index.html Outdated Show resolved Hide resolved
packages/web/src/scss/components/Skeleton/README.md Outdated Show resolved Hide resolved
@katerinarehorkova katerinarehorkova force-pushed the feat/ds-1625-skeleton-web branch from 9c234ee to df3917c Compare February 3, 2025 09:00
@literat
Copy link
Collaborator

literat commented Feb 3, 2025

It is looking good with the new tokens. Great work. I have only one question regarding the animation: Can it be more smooth, please? Now it looks like there is a little jump or hiccup between the end and the start of the loop.

I am not sure if this is understandable but if I compare the animation with the https://design-system.seduo.com/docs/components/skeleton, the animation starts and ends in the same position, e.g. no gradient is displayed. But in our case, we are starting with a displayed gradient at the start of the Skeleton and we end the ease with the displayed gradient at the end. So when the loop restarts there is a little "hiccup" in the animation.

🔝 @crishpeen But if we have agreed on this, maybe it is fine 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants