-
Notifications
You must be signed in to change notification settings - Fork 734
Update v0.9 Spec: Button variants and Text link #501
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
base: main
Are you sure you want to change the base?
Conversation
…Text.variant 'link'
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.
Code Review
This pull request updates the v0.9 specification by refactoring the Button component to use a variant property instead of a boolean primary property, and by adding a link variant to the Text component. The changes are consistent and improve the spec's flexibility.
My review includes two main points:
- The new
linkvariant for theTextcomponent is functionally incomplete as it lacks a way to define the link's target or action. I've suggested adding anactionproperty to make it a fully functional hyperlink. - The documentation for the
Button's newvariantproperty could be clearer. I've suggested an improved description that explains all variants and the default behavior.
| "type": "string", | ||
| "description": "A hint for the base text style.", | ||
| "enum": ["h1", "h2", "h3", "h4", "h5", "caption", "body"] | ||
| "enum": ["h1", "h2", "h3", "h4", "h5", "caption", "body", "link"] |
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.
Adding a link variant to the Text component is a great idea for semantic UI. However, in its current form, it only provides styling without any way to specify the link's destination or action. This makes it non-functional as a hyperlink.
To make this feature complete, I suggest adding an optional action property to the Text component, similar to how it's implemented for the Button component. This would allow developers to specify what happens when the link is clicked.
An example of how the Text component properties could be updated:
"properties": {
"component": { "const": "Text" },
"text": { ... },
"variant": { ... },
"action": {
"description": "The action to dispatch when the text is clicked. Should be used with variant 'link'.",
"$ref": "#/components/Button/allOf/3/properties/action"
}
}Additionally, the description for the text property (line 19) states that links are not supported in Markdown. This should be updated to clarify the role of the new link variant.
| "description": "Indicates if this button should be styled as the primary action." | ||
| "variant": { | ||
| "type": "string", | ||
| "description": "A hint for the button style. 'borderless' means the button adds no visual details, but just makes the content trigger an action.", |
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 description for the new variant property on the Button component is a bit sparse. It only explains the borderless variant and doesn't mention primary or what the default behavior is if the property is omitted.
To improve clarity for developers using this spec, I recommend expanding the description to cover all cases.
| "description": "A hint for the button style. 'borderless' means the button adds no visual details, but just makes the content trigger an action.", | |
| "description": "A hint for the button style. If omitted, a default button style is used. 'primary' indicates this is the main call-to-action button. 'borderless' means the button has no visual border or background, making its child content appear like a clickable link.", |
This PR updates the v0.9 specification to align the
Buttoncomponent with other components by replacing the booleanprimaryproperty with avariantproperty (supporting 'primary' and 'borderless'). It also adds a 'link' variant to theTextcomponent.