feat: add a possibility to use button component as link #327
Conversation
karinamaulitova
commented
Jul 22, 2022
karinamaulitova
commented
- update tests
+ update tests
packages/button/src/Button.tsx
Outdated
| @@ -35,6 +36,7 @@ function Button(props: ButtonProps, ref?: ForwardedRef<HTMLButtonElement>) { | |||
|
|
|||
| return ( | |||
| <ButtonStyle | |||
| as={(href ? 'a' : 'button') as React.ElementType} | |||
There was a problem hiding this comment.
is it possible to write href ? 'a' : null instead?
| @@ -46,6 +48,7 @@ function Button(props: ButtonProps, ref?: ForwardedRef<HTMLButtonElement>) { | |||
| disabled={disabled || loading} | |||
| type='button' | |||
| ref={ref} | |||
| href={href} | |||
There was a problem hiding this comment.
It's not the best way from types standpoint, because <button> props and <a> props are different:
So basically we need that button should accept button props, and link should accept link props
cc @Jabher for discussion
There was a problem hiding this comment.
What do you suggest in this situation?
There was a problem hiding this comment.
I personally like this approach, it is nice and minimal - by providing href you expect it to still be same component visually while making it a link. however, you are poiting different thing right - e.g. target attribute should be also supported here.
There was a problem hiding this comment.
yep, links and buttons have different props, and we probably should be able to specify which to use with link, otherwise there may be more prs and more if statements
| @@ -12,6 +13,8 @@ exports[`renders correctly 1`] = ` | |||
| background: transparent; | |||
| font-family: inherit; | |||
| font-weight: 700; | |||
| -webkit-text-decoration: none; | |||
There was a problem hiding this comment.
Don't we have any prefixer for this?
There was a problem hiding this comment.
It's a test snapshot, tests see prefixed code:)
| ...rest | ||
| } = props | ||
| } = props as ButtonButtonProps |
There was a problem hiding this comment.
We can add validator which type it is, so we don't cast it, it may help to use proper args further
There was a problem hiding this comment.
We have a disabled appearance of a button even if it is a link
There was a problem hiding this comment.
ButtonButton feels like this, maybe better naming can be here (e.g. NativeButton):
| @@ -11,7 +11,10 @@ export const useRipple: UseRipple = ({ onClick }) => { | |||
| const [ripple, setRipple] = useState<React.ReactNode | null>(null) | |||
|
|
|||
| const handleClick = useCallback( | |||
| (event: React.MouseEvent<HTMLButtonElement, MouseEvent>) => { | |||
| ( | |||
| event: React.MouseEvent<HTMLAnchorElement, MouseEvent> & | |||
There was a problem hiding this comment.
Why is it & here? It's probably anchor element | button element
There was a problem hiding this comment.
It works only with &
