Conversation
- Using all component tokens and styles that were previously-missing
Button changes ##BREAKING CHANGES - `skeleton` prop has been removed. Use `ButtonSkeleton` component instead - `size` prop has new values - `kind` prop has new values - `isSelected` is now `selected - `on:mouseover`, `on:mouseenter`, and `on:mouseleave` forwarded events were replaced with `on:pointerover`, `on:pointerenter`, and `on:pointerleave` - `as` no longer accepts boolean types ## Features - New `2xl` size - `expressive` styles now apply to all button sizes - `as` accepts a string to specify a desired HTML element
|
I'm accepting preliminary reviews until the tasks above are complete. |
gregorw
left a comment
gregorw
left a comment
There was a problem hiding this comment.
This is very hard to review due to the many changes.
Changes to Button using v11 can be much smaller and they still look good.
My 2 cents would be to separate v11 styles from feature parity. Otherwise it will be very hard to review the changes in next/v11-styles compared to master. We’re taking two steps at the same time and it will make reviewing, writing release notes and upgrading existing apps very very hard.
See also #1889 (comment).
If we really want to move the needle with v11 styles we should work on components such as Theme, Layer, CodeSnippet, maybe Stack, etc.
src/Button/Button.svelte
Outdated
| * @type {import('svelte/elements').HTMLAnchorAttributes | | ||
| * import('svelte/elements').HTMLButtonAttributes | import('svelte/elements').HTMLAttributes} | ||
| */ | ||
| export let buttonAttributes = {}; |
There was a problem hiding this comment.
I am not a fan of this, at least not in this form/for this component.
- It's unwieldy, you repeat "button" which is already part of the element, maybe it could just be
attributesif it refers to the root element Buttonprimarily is just a wrapper around a single element, I would prefer to pass any unknown properties automatically via$$restProps- If an attributes property is used, there should at least also be
iconAttributes. - It is inconsistent, some attributes, like
disabled, have their own property.
There was a problem hiding this comment.
These are all good points. I think it's fair to revise #1621 accordingly.
maybe it could just be
attributesif it refers to the root element
I think attributes works. The main benefit of a dedicated prop over $$restProps is for more ergonomic autocomplete. Only component props would appear in the IDE, and element attributes would be contained within a dedicated prop such as attributes.
I would prefer to pass any unknown properties automatically via
$$restProps
Given the autocomplete trade-off above, I'm leaning towards attributes, but I'm also torn because this:
<Button kind="secondary" type="submit" data-test-id="abc123">Save</button>Looks nicer than this:
<Button kind="secondary" type="submit" attributes={{ 'data-test-id': "abc123" }}>
Save
</button>there should at least also be
iconAttributes.
Agreed.
We have this note in our standardization ticket:
HTML attributes are provided via Type declarations and should not be included as explicit props unless it has special treatment as part of a component.
Appropriate:
disabledfor<Button>
Inappropriate:aria-labelfor<TextInput>
I suppose disabled has special treatment depending on usage of as or href. For that reason, I think it deserves to be a prop.
- Fix ButtonSkeleton sizes - Apply v11 `size` props
3 participants
Relates: #1641
For #1629
Meta
Todo
Out of scope
Button changes
BREAKING CHANGES
sizeprop has new values:sm,md,lg,xl, and2xl.xlis the default.kindprop has new values:danger-tertiaryis nowdanger--tertiary(two hyphens), anddanger-ghostis nowdanger--ghost(two hyphens)Features
2xlsizeexpressivestyles now apply to all button sizesFixes