From 49c44da73b15c4fd38fecd0f8eac19a0fa1c7832 Mon Sep 17 00:00:00 2001 From: Andrej Ocenas Date: Tue, 29 Oct 2019 09:39:44 +0100 Subject: [PATCH] Grafana/ui: Refactor button and add default type = button (#20042) --- .../src/components/Button/Button.story.tsx | 3 +- .../src/components/Button/Button.test.tsx | 26 +++++++ .../src/components/Button/Button.tsx | 54 +++++++++++++-- .../src/components/Button/ButtonContent.tsx | 20 ++++++ .../Button/__snapshots__/Button.test.tsx.snap | 5 ++ .../Button/{AbstractButton.tsx => styles.ts} | 68 +------------------ .../grafana-ui/src/components/Button/types.ts | 23 +------ .../src/components/Forms/Button.story.tsx | 6 +- .../src/components/Forms/Button.tsx | 57 +++++++++++----- 9 files changed, 148 insertions(+), 114 deletions(-) create mode 100644 packages/grafana-ui/src/components/Button/Button.test.tsx create mode 100644 packages/grafana-ui/src/components/Button/ButtonContent.tsx create mode 100644 packages/grafana-ui/src/components/Button/__snapshots__/Button.test.tsx.snap rename packages/grafana-ui/src/components/Button/{AbstractButton.tsx => styles.ts} (66%) diff --git a/packages/grafana-ui/src/components/Button/Button.story.tsx b/packages/grafana-ui/src/components/Button/Button.story.tsx index 3e528e8dc0f..b73a6419aec 100644 --- a/packages/grafana-ui/src/components/Button/Button.story.tsx +++ b/packages/grafana-ui/src/components/Button/Button.story.tsx @@ -5,7 +5,6 @@ import withPropsCombinations from 'react-storybook-addon-props-combinations'; import { action } from '@storybook/addon-actions'; import { ThemeableCombinationsRowRenderer } from '../../utils/storybook/CombinationsRowRenderer'; import { select, boolean } from '@storybook/addon-knobs'; -import { CommonButtonProps } from './types'; const ButtonStories = storiesOf('UI/Button', module); @@ -22,7 +21,7 @@ const combinationOptions = { CombinationRenderer: ThemeableCombinationsRowRenderer, }; -const renderButtonStory = (buttonComponent: React.ComponentType) => { +const renderButtonStory = (buttonComponent: typeof Button | typeof LinkButton) => { const isDisabled = boolean('Disable button', false); return withPropsCombinations( buttonComponent, diff --git a/packages/grafana-ui/src/components/Button/Button.test.tsx b/packages/grafana-ui/src/components/Button/Button.test.tsx new file mode 100644 index 00000000000..e71d84f43bf --- /dev/null +++ b/packages/grafana-ui/src/components/Button/Button.test.tsx @@ -0,0 +1,26 @@ +import React from 'react'; +import { Button, LinkButton } from './Button'; +import { mount } from 'enzyme'; + +describe('Button', () => { + it('renders correct html', () => { + const wrapper = mount(); + expect(wrapper.html()).toMatchSnapshot(); + }); +}); + +describe('LinkButton', () => { + it('renders correct html', () => { + const wrapper = mount(Click me); + expect(wrapper.html()).toMatchSnapshot(); + }); + + it('allows a disable state on link button', () => { + const wrapper = mount( + + Click me + + ); + expect(wrapper.find('a[disabled]').length).toBe(1); + }); +}); diff --git a/packages/grafana-ui/src/components/Button/Button.tsx b/packages/grafana-ui/src/components/Button/Button.tsx index b9db2ed1447..3632059c547 100644 --- a/packages/grafana-ui/src/components/Button/Button.tsx +++ b/packages/grafana-ui/src/components/Button/Button.tsx @@ -1,16 +1,60 @@ -import React, { useContext } from 'react'; -import { AbstractButton } from './AbstractButton'; +import React, { AnchorHTMLAttributes, ButtonHTMLAttributes, useContext } from 'react'; import { ThemeContext } from '../../themes'; -import { ButtonProps, LinkButtonProps } from './types'; +import { getButtonStyles } from './styles'; +import { ButtonContent } from './ButtonContent'; +import cx from 'classnames'; +import { ButtonSize, ButtonStyles, ButtonVariant } from './types'; +type CommonProps = { + size?: ButtonSize; + variant?: ButtonVariant; + /** + * icon prop is a temporary solution. It accepts legacy icon class names for the icon to be rendered. + * TODO: migrate to a component when we are going to migrate icons to @grafana/ui + */ + icon?: string; + className?: string; + styles?: ButtonStyles; +}; + +type ButtonProps = CommonProps & ButtonHTMLAttributes; export const Button: React.FunctionComponent = props => { const theme = useContext(ThemeContext); - return ; + const { size, variant, icon, children, className, styles: stylesProp, ...buttonProps } = props; + + // Default this to 'button', otherwise html defaults to 'submit' which then submits any form it is in. + buttonProps.type = buttonProps.type || 'button'; + const styles = + stylesProp || getButtonStyles({ theme, size: size || 'md', variant: variant || 'primary', withIcon: !!icon }); + + return ( + + ); }; Button.displayName = 'Button'; +type LinkButtonProps = CommonProps & + AnchorHTMLAttributes & { + // We allow disabled here even though it is not standard for a link. We use it as a selector to style it as + // disabled. + disabled?: boolean; + }; export const LinkButton: React.FunctionComponent = props => { const theme = useContext(ThemeContext); - return ; + const { size, variant, icon, children, className, styles: stylesProp, ...anchorProps } = props; + const styles = + stylesProp || getButtonStyles({ theme, size: size || 'md', variant: variant || 'primary', withIcon: !!icon }); + + return ( + + + {children} + + + ); }; LinkButton.displayName = 'LinkButton'; diff --git a/packages/grafana-ui/src/components/Button/ButtonContent.tsx b/packages/grafana-ui/src/components/Button/ButtonContent.tsx new file mode 100644 index 00000000000..f178350a965 --- /dev/null +++ b/packages/grafana-ui/src/components/Button/ButtonContent.tsx @@ -0,0 +1,20 @@ +import React from 'react'; +import cx from 'classnames'; + +type Props = { + icon?: string; + className: string; + iconClassName: string; + children: React.ReactNode; +}; +export function ButtonContent(props: Props) { + const { icon, className, iconClassName, children } = props; + return icon ? ( + + + {children} + + ) : ( + <>{children} + ); +} diff --git a/packages/grafana-ui/src/components/Button/__snapshots__/Button.test.tsx.snap b/packages/grafana-ui/src/components/Button/__snapshots__/Button.test.tsx.snap new file mode 100644 index 00000000000..c4cd245cc09 --- /dev/null +++ b/packages/grafana-ui/src/components/Button/__snapshots__/Button.test.tsx.snap @@ -0,0 +1,5 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`Button renders correct html 1`] = `""`; + +exports[`LinkButton renders correct html 1`] = `"Click me"`; diff --git a/packages/grafana-ui/src/components/Button/AbstractButton.tsx b/packages/grafana-ui/src/components/Button/styles.ts similarity index 66% rename from packages/grafana-ui/src/components/Button/AbstractButton.tsx rename to packages/grafana-ui/src/components/Button/styles.ts index 8a8467174e3..66b5112002a 100644 --- a/packages/grafana-ui/src/components/Button/AbstractButton.tsx +++ b/packages/grafana-ui/src/components/Button/styles.ts @@ -1,9 +1,7 @@ -import React, { ComponentType, ReactNode } from 'react'; import tinycolor from 'tinycolor2'; -import { css, cx } from 'emotion'; +import { css } from 'emotion'; import { selectThemeVariant, stylesFactory } from '../../themes'; -import { AbstractButtonProps, ButtonSize, ButtonStyles, ButtonVariant, CommonButtonProps, StyleDeps } from './types'; -import { GrafanaTheme } from '../../types'; +import { StyleDeps } from './types'; const buttonVariantStyles = ( from: string, @@ -26,7 +24,7 @@ const buttonVariantStyles = ( } `; -const getButtonStyles = stylesFactory(({ theme, size, variant, withIcon }: StyleDeps) => { +export const getButtonStyles = stylesFactory(({ theme, size, variant, withIcon }: StyleDeps) => { const borderRadius = theme.border.radius.sm; let padding, background, @@ -140,63 +138,3 @@ const getButtonStyles = stylesFactory(({ theme, size, variant, withIcon }: Style `, }; }); - -export const renderButton = ( - theme: GrafanaTheme, - buttonStyles: ButtonStyles, - renderAs: ComponentType | string, - children: ReactNode, - size: ButtonSize, - variant: ButtonVariant, - icon?: string, - className?: string, - otherProps?: Partial -) => { - const nonHtmlProps = { - theme, - size, - variant, - }; - const finalClassName = cx(buttonStyles.button, className); - const finalChildren = icon ? ( - - - {children} - - ) : ( - children - ); - - const finalProps = - typeof renderAs === 'string' - ? { - ...otherProps, - className: finalClassName, - children: finalChildren, - } - : { - ...otherProps, - ...nonHtmlProps, - className: finalClassName, - children: finalChildren, - }; - - return React.createElement(renderAs, finalProps); -}; - -export const AbstractButton: React.FunctionComponent = ({ - renderAs, - theme, - size = 'md', - variant = 'primary', - className, - icon, - children, - ...otherProps -}) => { - const buttonStyles = getButtonStyles({ theme, size, variant, withIcon: !!icon }); - - return renderButton(theme, buttonStyles, renderAs, children, size, variant, icon, className, otherProps); -}; - -AbstractButton.displayName = 'AbstractButton'; diff --git a/packages/grafana-ui/src/components/Button/types.ts b/packages/grafana-ui/src/components/Button/types.ts index 5e719c5bad2..96385d68148 100644 --- a/packages/grafana-ui/src/components/Button/types.ts +++ b/packages/grafana-ui/src/components/Button/types.ts @@ -1,5 +1,4 @@ -import { AnchorHTMLAttributes, ButtonHTMLAttributes, ComponentType } from 'react'; -import { GrafanaTheme, Themeable } from '../../types'; +import { GrafanaTheme } from '../../types'; export type ButtonVariant = 'primary' | 'secondary' | 'danger' | 'inverse' | 'transparent' | 'destructive'; @@ -17,23 +16,3 @@ export interface ButtonStyles { iconWrap: string; icon: string; } - -export interface CommonButtonProps { - size?: ButtonSize; - variant?: ButtonVariant; - /** - * icon prop is a temporary solution. It accepts legacy icon class names for the icon to be rendered. - * TODO: migrate to a component when we are going to migrate icons to @grafana/ui - */ - icon?: string; - className?: string; -} - -export interface LinkButtonProps extends CommonButtonProps, AnchorHTMLAttributes { - disabled?: boolean; -} -export interface ButtonProps extends CommonButtonProps, ButtonHTMLAttributes {} - -export interface AbstractButtonProps extends CommonButtonProps, Themeable { - renderAs: ComponentType | string; -} diff --git a/packages/grafana-ui/src/components/Forms/Button.story.tsx b/packages/grafana-ui/src/components/Forms/Button.story.tsx index 6ca661448d1..f7abae1fa2e 100644 --- a/packages/grafana-ui/src/components/Forms/Button.story.tsx +++ b/packages/grafana-ui/src/components/Forms/Button.story.tsx @@ -1,8 +1,8 @@ import React from 'react'; -import { Button } from './Button'; +import { Button, ButtonVariant } from './Button'; import { withCenteredStory, withHorizontallyCenteredStory } from '../../utils/storybook/withCenteredStory'; import { select, text } from '@storybook/addon-knobs'; -import { ButtonSize, ButtonVariant } from '../Button/types'; +import { ButtonSize } from '../Button/types'; import mdx from './Button.mdx'; export default { @@ -26,7 +26,7 @@ export const simple = () => { const buttonText = text('text', 'Button'); return ( - ); diff --git a/packages/grafana-ui/src/components/Forms/Button.tsx b/packages/grafana-ui/src/components/Forms/Button.tsx index 6dcbdac2e3b..c1dac36551c 100644 --- a/packages/grafana-ui/src/components/Forms/Button.tsx +++ b/packages/grafana-ui/src/components/Forms/Button.tsx @@ -1,10 +1,10 @@ -import { FC } from 'react'; +import React, { AnchorHTMLAttributes, ButtonHTMLAttributes, useContext } from 'react'; import { css, cx } from 'emotion'; import tinycolor from 'tinycolor2'; -import { selectThemeVariant, stylesFactory, useTheme } from '../../themes'; -import { renderButton } from '../Button/AbstractButton'; +import { selectThemeVariant, stylesFactory, ThemeContext } from '../../themes'; +import { Button as DefaultButton, LinkButton as DefaultLinkButton } from '../Button/Button'; import { getFocusStyle } from './commonStyles'; -import { AbstractButtonProps, ButtonSize, ButtonVariant, StyleDeps } from '../Button/types'; +import { ButtonSize, StyleDeps } from '../Button/types'; import { GrafanaTheme } from '../../types'; const buttonVariantStyles = (from: string, to: string, textColor: string) => css` @@ -96,7 +96,9 @@ const getPropertiesForVariant = (theme: GrafanaTheme, variant: ButtonVariant) => } }; -export const getButtonStyles = stylesFactory(({ theme, size, variant, withIcon }: StyleDeps) => { +// Need to do this because of mismatch between variants in standard buttons and here +type StyleProps = Omit & { variant: ButtonVariant }; +export const getButtonStyles = stylesFactory(({ theme, size, variant, withIcon }: StyleProps) => { const { padding, fontSize, iconDistance, height } = getPropertiesForSize(theme, size); const { background, borderColor } = getPropertiesForVariant(theme, variant); @@ -142,17 +144,38 @@ export const getButtonStyles = stylesFactory(({ theme, size, variant, withIcon } }; }); -export const Button: FC> = ({ - renderAs, - size = 'md', - variant = 'primary', - className, - icon, - children, - ...otherProps -}) => { - const theme = useTheme(); - const buttonStyles = getButtonStyles({ theme, size, variant, withIcon: !!icon }); +// These are different from the standard Button where there are 5 variants. +export type ButtonVariant = 'primary' | 'secondary' | 'destructive'; - return renderButton(theme, buttonStyles, renderAs, children, size, variant, icon, className, otherProps); +// These also needs to be different because the ButtonVariant is different +type CommonProps = { + size?: ButtonSize; + variant?: ButtonVariant; + icon?: string; + className?: string; +}; + +type ButtonProps = CommonProps & ButtonHTMLAttributes; + +export const Button = (props: ButtonProps) => { + const theme = useContext(ThemeContext); + const styles = getButtonStyles({ + theme, + size: props.size || 'md', + variant: props.variant || 'primary', + withIcon: !!props.icon, + }); + return ; +}; + +type ButtonLinkProps = CommonProps & AnchorHTMLAttributes; +export const LinkButton = (props: ButtonLinkProps) => { + const theme = useContext(ThemeContext); + const styles = getButtonStyles({ + theme, + size: props.size || 'md', + variant: props.variant || 'primary', + withIcon: !!props.icon, + }); + return ; };