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

[antd5] add Skeleton component #103

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

georg-malahov
Copy link

Add support for Skeleton component.

Copy link

vercel bot commented Nov 28, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
react-email-demo ⬜️ Ignored (Inspect) Dec 19, 2023 5:05pm

@IcaroG
Copy link
Contributor

IcaroG commented Dec 19, 2023

Hey @georg-malahov.
Any reason for the change from defaultValueHint to defaultValue?
Usually we prefer to use defaultValueHint if the value is already the default value of the prop.

@georg-malahov
Copy link
Author

georg-malahov commented Dec 19, 2023

@IcaroG Yes, if am not mistaken now, by using defaultValueHint without providing exact defaultValue the conditional hidden property didn't work as expected. E.g. hidden: (ps) => ps.type !== "Avatar" && ps.avatar !== true, seemed to have no real values set in ps object.

name: skeletonComponentName,
displayName: 'Skeleton',
props: {
children: 'slot',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be hidden when type !== "Node"

type: {
type: 'choice',
defaultValue: 'Basic',
options: ['Basic', 'Button', 'Avatar', 'Input', 'Image', 'Node'],
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be some helpText for Node type (e.g. please use the children slot to add a node)

description: 'Show animation effect',
defaultValue: false,
},
avatar: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Avatar placeholder needs some margin on top and left

hidden: (ps) => ps.type !== 'Basic',
defaultValue: false,
},
loading: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be true by default, so that when you drag the Skeleton to the canvas, it shows something.

widthTitle: {
type: 'string',
description: 'Width of the title',
hidden: (ps) => !ps.title,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be hidden for all types other than Basic

hidden: (ps) => !ps.paragraph && ps.type !== 'Basic',
advanced: true,
},
rows: {
Copy link
Contributor

Choose a reason for hiding this comment

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

SHould have a defaultValueHint of 1

advanced: true,
},
// SkeletonButtonProps
shapeButton: {
Copy link
Contributor

Choose a reason for hiding this comment

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

This does nothing. Also, there's already the shape prop that works for this purpose

customClassName: {
type: 'string',
displayName: 'Class Name',
description: 'Custom class name for Node skeleton for custom styling',
Copy link
Contributor

Choose a reason for hiding this comment

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

Better use class prop type for this.

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

Successfully merging this pull request may close these issues.

3 participants