-
Notifications
You must be signed in to change notification settings - Fork 3
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
Feature/custom button #625
Changes from 23 commits
ee6d0d7
334431a
1c629d5
ad19491
2a6b563
f24d313
58c7ea1
24fd57e
cd63f17
4cb5dec
eabc972
d2f0662
68cbed4
e7b159c
857baef
c3e87d7
eaf84b3
603b4fa
c9c58b1
69cd8ef
7000cd9
625f288
ca5eba0
54580dd
ace0823
d504830
2db65f8
8da822e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,210 @@ | ||
import React, { ReactNode, useState, useEffect, forwardRef } from "react"; | ||
import { Button as AntButton, Tooltip } from "antd"; | ||
import styled, { css, RuleSet } from "styled-components"; | ||
import type { ButtonProps as AntButtonProps } from "antd"; | ||
import { | ||
NAV_BAR_TOOLTIP_OFFSET, | ||
TOOLTIP_COLOR, | ||
TOOLTIP_DELAY, | ||
} from "../../constants"; | ||
import { ButtonClass, TooltipPlacement } from "../../constants/interfaces"; | ||
|
||
interface CustomButtonProps extends Omit<AntButtonProps, "type" | "variant"> { | ||
variant?: ButtonClass; | ||
titleText?: string; | ||
icon?: ReactNode; | ||
onClick?: () => void; | ||
} | ||
|
||
interface TooltipText { | ||
defaultText: string; | ||
disabledText?: string; | ||
} | ||
|
||
interface TooltilButtonProps extends CustomButtonProps { | ||
tooltipText: TooltipText; | ||
tooltipPlacement?: TooltipPlacement; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently the style rules, basic button, and button with tooltip are all here in the same file. I'm open to the argument that they be split up! But at this stage I like having it all in one place. |
||
|
||
const baseStyles = css` | ||
font-family: ${(props) => props.theme.typography}; | ||
border-radius: 3px; | ||
height: 32px; | ||
padding: 6px 16px; | ||
font-size: 14px; | ||
align-items: center; | ||
gap: 8px; | ||
cursor: pointer; | ||
|
||
&:disabled { | ||
cursor: not-allowed; | ||
} | ||
|
||
&:focus-visible { | ||
outline: 1px solid ${({ theme }) => theme.colors.lightPurpleBg}; | ||
outline-offset: 1px; | ||
} | ||
`; | ||
|
||
const generateButtonStyles = ( | ||
variant: "primary" | "secondary", | ||
theme: "light" | "dark" | ||
) => css` | ||
${({ | ||
theme: { | ||
colors: { button }, | ||
}, | ||
}) => { | ||
const buttonTheme = button[variant][theme]; | ||
const { background, text, border, hover, active, disabled } = | ||
buttonTheme; | ||
|
||
return css` | ||
background-color: ${background}; | ||
border: 1px solid ${variant === "primary" ? background : border}; | ||
color: ${text}; | ||
|
||
&&& { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeesh. Which brand of CSS magic does this triple ampersand belong to? Does this mean "parent-parent-parent"..? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No! It's both better and worse than that. In Because some When our styles don't really map onto ant's opinions very well, I prefer to just sort of veto all their styling by using this triple ampersand. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I sort of it get it now. Does there come a point where we eschew the ant buttons altogether and just roll our own from scratch 🤔 ? I'd be curious as to when the team would consider that to be worth it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am trending towards a "I want to build it from scratch mindset" but also acknowledge the many benefits that do come form using this library. I think migrations like this are a point where we have to "pay" the deferred update cost that we would be paying in ongoing way when maintaining our own components. The buttons may have reached a place where I would consider getting rid of the ant component, but in general ant does save us a lot of dev time. |
||
&:hover:not(:disabled) { | ||
background-color: ${hover.background}; | ||
border-color: ${hover.background}; | ||
color: ${hover.text}; | ||
} | ||
|
||
&:active:not(:disabled) { | ||
background-color: ${hover.background}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I feel like I'd expect this to be |
||
border-color: ${active.background}; | ||
color: ${active.text}; | ||
} | ||
|
||
&:disabled { | ||
background-color: ${disabled.background}; | ||
border-color: ${disabled.background}; | ||
color: ${disabled.text}; | ||
} | ||
} | ||
`; | ||
}} | ||
`; | ||
|
||
const actionStyles = css` | ||
${({ | ||
theme: { | ||
colors: { | ||
button: { action }, | ||
}, | ||
}, | ||
}) => css` | ||
background: ${action.background}; | ||
border: none; | ||
color: ${action.text}; | ||
padding: 6px 8px; | ||
min-width: auto; | ||
|
||
&&& { | ||
&:hover:not(:disabled) { | ||
color: ${action.hover.background}; | ||
} | ||
|
||
&:active:not(:disabled) { | ||
border: ${action.active.text}; | ||
color: ${action.active.text}; | ||
} | ||
|
||
&:disabled { | ||
color: ${action.disabled.text}; | ||
} | ||
} | ||
`} | ||
`; | ||
|
||
const variantStyles: Record<ButtonClass, RuleSet<object>> = { | ||
[ButtonClass.LightPrimary]: generateButtonStyles("primary", "light"), | ||
[ButtonClass.LightSecondary]: generateButtonStyles("secondary", "light"), | ||
[ButtonClass.DarkPrimary]: generateButtonStyles("primary", "dark"), | ||
[ButtonClass.DarkSecondary]: generateButtonStyles("secondary", "dark"), | ||
[ButtonClass.Action]: actionStyles, | ||
}; | ||
|
||
const StyledButton = styled(AntButton)<CustomButtonProps>` | ||
${baseStyles} | ||
${({ variant = ButtonClass.LightPrimary }) => variantStyles[variant]} | ||
`; | ||
|
||
export const CustomButton = forwardRef<HTMLButtonElement, CustomButtonProps>( | ||
( | ||
{ | ||
children, | ||
variant = ButtonClass.LightPrimary, | ||
titleText, | ||
icon, | ||
onClick, | ||
disabled, | ||
...props | ||
}, | ||
ref | ||
) => { | ||
return ( | ||
<StyledButton | ||
ref={ref} | ||
variant={variant} | ||
onClick={onClick} | ||
disabled={disabled} | ||
{...props} | ||
> | ||
{titleText || children} {icon} | ||
</StyledButton> | ||
); | ||
} | ||
); | ||
|
||
CustomButton.displayName = "CustomButton"; | ||
|
||
export const TooltipButton: React.FC<TooltilButtonProps> = ({ | ||
tooltipText = { defaultText: "", disabledText: "" }, | ||
tooltipPlacement, | ||
disabled = false, | ||
...buttonProps | ||
}) => { | ||
const [tooltipRenderText, setTooltipRenderText] = useState( | ||
disabled ? tooltipText.disabledText : tooltipText.defaultText | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need to be stateful? Is this to prevent the rendered text from changing while the tooltip is open? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey good catch, no it doesn't. Also the tooltip behavior wasn't actually working with the new disabled prop so this comment helped me catch that! Adding a wrapper div to handle the mouse events so disabled buttons still show tooltips, and making |
||
const [tooltipVisible, setTooltipVisible] = useState(false); | ||
|
||
useEffect(() => { | ||
if (!tooltipVisible) { | ||
setTooltipRenderText( | ||
disabled ? tooltipText.disabledText : tooltipText.defaultText | ||
); | ||
} | ||
}, [disabled, tooltipText, tooltipVisible]); | ||
|
||
const handleMouseEnter = (e: React.MouseEvent<HTMLElement>) => { | ||
setTooltipVisible(true); | ||
buttonProps.onMouseEnter?.(e); | ||
}; | ||
|
||
const handleMouseLeave = (e: React.MouseEvent<HTMLElement>) => { | ||
setTooltipVisible(false); | ||
buttonProps.onMouseLeave?.(e); | ||
}; | ||
|
||
return ( | ||
<Tooltip | ||
placement={tooltipPlacement} | ||
title={tooltipRenderText} | ||
color={TOOLTIP_COLOR} | ||
mouseEnterDelay={TOOLTIP_DELAY} | ||
align={{ targetOffset: NAV_BAR_TOOLTIP_OFFSET }} | ||
trigger={["hover", "focus"]} | ||
open={tooltipVisible} | ||
> | ||
<CustomButton | ||
{...buttonProps} | ||
disabled={disabled} | ||
onMouseEnter={handleMouseEnter} | ||
onMouseLeave={handleMouseLeave} | ||
/> | ||
</Tooltip> | ||
); | ||
}; |
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.