Skip to content
This repository has been archived by the owner on Aug 7, 2024. It is now read-only.

feat : Button component onClick and href prop #10156

Closed
wants to merge 1 commit into from
Closed

feat : Button component onClick and href prop #10156

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions components/Button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ export default function Button({
disabled = false,
className,
overrideClassNames = false,
href,
onClick,
children,
...restProps
}) {
Expand Down Expand Up @@ -34,6 +36,8 @@ export default function Button({

const button = (
<button
href={href}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
href={href}

Shouldn't we move the href prop to Link?
It seems like the intention was to use the href prop to determine whether to render a Link or a button. However, the return statement (return restProps.href ? link : button;) will always return a link (without an href) in this case.

Maybe we should move the href prop to the Link component. This way, when href is present, the Link component will be rendered with the href attribute, and when href is not present, a button will be rendered instead.

<Link
      href={href}

Also we should change the return statement to href ? link : button;

Copy link
Author

Choose a reason for hiding this comment

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

i was thinking of the same thing as well

onClick={onClick}
className={
overrideClassNames ? className : classNames(defaultClassName, className)
}
Expand Down
Loading