-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
pr05 Typescript Migration #5: Migrate client/common folder #3565
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
base: develop
Are you sure you want to change the base?
Changes from all commits
f1af0a6
8a0f302
1cb8f5b
d376207
29e1e6d
1efe93d
6042638
c1883c5
eb69138
5dd03d1
197fd67
cfb4b82
d80961c
24368cc
e67e015
1b5373c
b5eea95
7ca1e6d
7075ffb
6bc9369
94ea59a
1b72ea4
7ce4f54
7188c1b
bee3211
6148c33
3e8faea
4bd9443
7718e02
3428dac
1a7cfaa
3676534
366aa7e
2d8fe06
b1788d5
bc82be5
7b67549
379e563
9515291
dc7665d
0ff9ac1
a09c8d2
7e7c1ab
67425a1
001c9e4
e8ba7b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| import React from 'react'; | ||
| import { render, screen, fireEvent } from '../test-utils'; | ||
| import { Button } from './Button'; | ||
|
|
||
| const MockIcon = (props: React.SVGProps<SVGSVGElement>) => ( | ||
| <svg data-testid="mock-icon" {...props} /> | ||
| ); | ||
|
|
||
| describe('Button', () => { | ||
| // Tag | ||
| it('renders as an anchor when href is provided', () => { | ||
| render(<Button href="https://example.com">Link</Button>); | ||
| const anchor = screen.getByRole('link'); | ||
| expect(anchor.tagName.toLowerCase()).toBe('a'); | ||
| expect(anchor).toHaveAttribute('href', 'https://example.com'); | ||
| }); | ||
|
|
||
| it('renders as a React Router <Link> when `to` is provided', () => { | ||
| render(<Button to="/dashboard">Go</Button>); | ||
| const link = screen.getByRole('link'); | ||
| expect(link.tagName.toLowerCase()).toBe('a'); // Link renders as <a> | ||
| expect(link).toHaveAttribute('href', '/dashboard'); | ||
| }); | ||
|
|
||
| it('renders as a <button> with a type of "button" by default', () => { | ||
| render(<Button>Click Me</Button>); | ||
| const el = screen.getByRole('button'); | ||
| expect(el.tagName.toLowerCase()).toBe('button'); | ||
| expect(el).toHaveAttribute('type', 'button'); | ||
| }); | ||
|
|
||
| // Children & Icons | ||
| it('renders children', () => { | ||
| render(<Button>Click Me</Button>); | ||
| expect(screen.getByText('Click Me')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('renders an iconBefore and button text', () => { | ||
| render( | ||
| <Button iconBefore={<MockIcon aria-label="iconbefore" />}> | ||
| This has a before icon | ||
| </Button> | ||
| ); | ||
| expect(screen.getByLabelText('iconbefore')).toBeInTheDocument(); | ||
| expect(screen.getByRole('button')).toHaveTextContent( | ||
| 'This has a before icon' | ||
| ); | ||
| }); | ||
|
|
||
| it('renders with iconAfter', () => { | ||
| render( | ||
| <Button iconAfter={<MockIcon aria-label="iconafter" />}> | ||
| This has an after icon | ||
| </Button> | ||
| ); | ||
| expect(screen.getByLabelText('iconafter')).toBeInTheDocument(); | ||
| expect(screen.getByRole('button')).toHaveTextContent( | ||
| 'This has an after icon' | ||
| ); | ||
| }); | ||
|
|
||
| it('renders only the icon if iconOnly', () => { | ||
| render( | ||
| <Button iconAfter={<MockIcon aria-label="iconafter" />} iconOnly> | ||
| This has an after icon | ||
| </Button> | ||
| ); | ||
| expect(screen.getByLabelText('iconafter')).toBeInTheDocument(); | ||
| expect(screen.getByRole('button')).not.toHaveTextContent( | ||
| 'This has an after icon' | ||
| ); | ||
| }); | ||
|
|
||
| // HTML attributes | ||
| it('calls onClick handler when clicked', () => { | ||
| const handleClick = jest.fn(); | ||
| render(<Button onClick={handleClick}>Click</Button>); | ||
| fireEvent.click(screen.getByText('Click')); | ||
| expect(handleClick).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('renders disabled state', () => { | ||
| render(<Button disabled>Disabled</Button>); | ||
| expect(screen.getByRole('button')).toBeDisabled(); | ||
| }); | ||
|
|
||
| it('uses aria-label when provided', () => { | ||
| render(<Button aria-label="Upload" iconOnly />); | ||
| expect(screen.getByLabelText('Upload')).toBeInTheDocument(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,28 +1,93 @@ | ||
| import React from 'react'; | ||
| import PropTypes from 'prop-types'; | ||
| import styled from 'styled-components'; | ||
| import { Link } from 'react-router-dom'; | ||
|
|
||
| import { remSize, prop } from '../theme'; | ||
|
|
||
| const kinds = { | ||
| primary: 'primary', | ||
| secondary: 'secondary' | ||
| }; | ||
| enum Kinds { | ||
| primary = 'primary', | ||
| secondary = 'secondary' | ||
| } | ||
|
|
||
| const displays = { | ||
| block: 'block', | ||
| inline: 'inline' | ||
| }; | ||
| enum Displays { | ||
| block = 'block', | ||
| inline = 'inline' | ||
| } | ||
|
|
||
| enum ButtonTypes { | ||
| button = 'button', | ||
| submit = 'submit' | ||
| } | ||
|
|
||
| export interface ButtonProps extends React.HTMLAttributes<HTMLElement> { | ||
| /** | ||
| * The visible part of the button, telling the user what | ||
| * the action is | ||
| */ | ||
| children?: React.ReactNode; | ||
| /** | ||
| If the button can be activated or not | ||
| */ | ||
| disabled?: boolean; | ||
| /** | ||
| * The display type of the button—inline or block | ||
| */ | ||
| display?: Displays; | ||
| /** | ||
| * SVG icon to place after child content | ||
| */ | ||
| iconAfter?: React.ReactNode; | ||
| /** | ||
| * SVG icon to place before child content | ||
| */ | ||
| iconBefore?: React.ReactNode; | ||
| /** | ||
| * If the button content is only an SVG icon | ||
| */ | ||
| iconOnly?: boolean; | ||
| /** | ||
| * The kind of button - determines how it appears visually | ||
| */ | ||
| kind?: Kinds; | ||
| /** | ||
| * Specifying an href will use an <a> to link to the URL | ||
| */ | ||
| href?: string; | ||
| /** | ||
| * An ARIA Label used for accessibility | ||
| */ | ||
| 'aria-label'?: string; | ||
| /** | ||
| * Specifying a to URL will use a react-router Link | ||
| */ | ||
| to?: string; | ||
| /** | ||
| * If using a native button, specifies on an onClick action | ||
| */ | ||
| onClick?: () => void; | ||
| /** | ||
| * If using a button, then type is defines the type of button | ||
| */ | ||
| type?: ButtonTypes; | ||
| /** | ||
| * Allows for IconButton to pass `focusable="false"` as a prop for SVGs. | ||
| * See @types/react > interface SVGAttributes<T> extends AriaAttributes, DOMAttributes<T> | ||
| */ | ||
| focusable?: boolean | 'true' | 'false'; | ||
|
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. can we have it just be the boolean type? if the parent that is using this component has the information stored as a string we should have it be the parents' responsibility to cast it to a boolean type before it's passed in 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. bump! |
||
| } | ||
|
|
||
| interface StyledButtonProps extends ButtonProps { | ||
| kind: Kinds; | ||
| display: Displays; | ||
| } | ||
|
|
||
| // The '&&&' will increase the specificity of the | ||
| // component's CSS so that it overrides the more | ||
| // general global styles | ||
| const StyledButton = styled.button` | ||
| const StyledButton = styled.button<StyledButtonProps>` | ||
| &&& { | ||
| font-weight: bold; | ||
| display: ${({ display }) => | ||
| display === displays.inline ? 'inline-flex' : 'flex'}; | ||
| display === Displays.inline ? 'inline-flex' : 'flex'}; | ||
| justify-content: center; | ||
| align-items: center; | ||
|
|
||
|
|
@@ -109,35 +174,37 @@ const StyledInlineButton = styled.button` | |
| `; | ||
|
|
||
| /** | ||
| * A Button performs an primary action | ||
| * Renders a component with a button appearance, but which is: | ||
| * - External anchor link if passed a `href` | ||
| * - Internal React Router link if passed a `to` | ||
| * - Default: Native Button | ||
| */ | ||
| const Button = ({ | ||
| children, | ||
| display, | ||
| export const Button = ({ | ||
| children = null, | ||
| display = Displays.block, | ||
| href, | ||
| kind, | ||
| iconBefore, | ||
| iconAfter, | ||
| iconOnly, | ||
| kind = Kinds.primary, | ||
| iconBefore = null, | ||
| iconAfter = null, | ||
| iconOnly = false, | ||
| 'aria-label': ariaLabel, | ||
| to, | ||
| type, | ||
| type = ButtonTypes.button, | ||
| ...props | ||
| }) => { | ||
| }: ButtonProps) => { | ||
| const hasChildren = React.Children.count(children) > 0; | ||
| const content = ( | ||
| <> | ||
| {iconBefore} | ||
| {hasChildren && <span>{children}</span>} | ||
| {hasChildren && !iconOnly && <span>{children}</span>} | ||
| {iconAfter} | ||
| </> | ||
| ); | ||
| let StyledComponent = StyledButton; | ||
|
|
||
| if (iconOnly) { | ||
| StyledComponent = StyledInlineButton; | ||
| } | ||
| const StyledComponent: React.ElementType = iconOnly | ||
| ? StyledInlineButton | ||
| : StyledButton; | ||
|
|
||
| // Anchor Link | ||
| if (href) { | ||
| return ( | ||
| <StyledComponent | ||
|
|
@@ -153,6 +220,7 @@ const Button = ({ | |
| ); | ||
| } | ||
|
|
||
| // Internal React Router Link | ||
| if (to) { | ||
| return ( | ||
| <StyledComponent | ||
|
|
@@ -168,6 +236,7 @@ const Button = ({ | |
| ); | ||
| } | ||
|
|
||
| // Native Button | ||
| return ( | ||
| <StyledComponent | ||
| kind={kind} | ||
|
|
@@ -181,69 +250,5 @@ const Button = ({ | |
| ); | ||
| }; | ||
|
|
||
| Button.defaultProps = { | ||
|
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. it's so great that we can get rid of all of this extra code at the bottom of the React files now haha, thanks for doing this conversion!! |
||
| children: null, | ||
| disabled: false, | ||
| display: displays.block, | ||
| iconAfter: null, | ||
| iconBefore: null, | ||
| iconOnly: false, | ||
| kind: kinds.primary, | ||
| href: null, | ||
| 'aria-label': null, | ||
| to: null, | ||
| type: 'button' | ||
| }; | ||
|
|
||
| Button.kinds = kinds; | ||
| Button.displays = displays; | ||
|
|
||
| Button.propTypes = { | ||
| /** | ||
| * The visible part of the button, telling the user what | ||
| * the action is | ||
| */ | ||
| children: PropTypes.oneOfType([PropTypes.element, PropTypes.string]), | ||
| /** | ||
| If the button can be activated or not | ||
| */ | ||
| disabled: PropTypes.bool, | ||
| /** | ||
| * The display type of the button—inline or block | ||
| */ | ||
| display: PropTypes.oneOf(Object.values(displays)), | ||
| /** | ||
| * SVG icon to place after child content | ||
| */ | ||
| iconAfter: PropTypes.element, | ||
| /** | ||
| * SVG icon to place before child content | ||
| */ | ||
| iconBefore: PropTypes.element, | ||
| /** | ||
| * If the button content is only an SVG icon | ||
| */ | ||
| iconOnly: PropTypes.bool, | ||
| /** | ||
| * The kind of button - determines how it appears visually | ||
| */ | ||
| kind: PropTypes.oneOf(Object.values(kinds)), | ||
| /** | ||
| * Specifying an href will use an <a> to link to the URL | ||
| */ | ||
| href: PropTypes.string, | ||
| /** | ||
| * An ARIA Label used for accessibility | ||
| */ | ||
| 'aria-label': PropTypes.string, | ||
| /** | ||
| * Specifying a to URL will use a react-router Link | ||
| */ | ||
| to: PropTypes.string, | ||
| /** | ||
| * If using a button, then type is defines the type of button | ||
| */ | ||
| type: PropTypes.oneOf(['button', 'submit']) | ||
| }; | ||
|
|
||
| export default Button; | ||
| Button.kinds = Kinds; | ||
|
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. feels like instead of doing this, we could just export Kinds and Displays? like
and then other files can do this:
|
||
| Button.displays = Displays; | ||
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.
i think enums should use constant case for the fields! https://google.github.io/styleguide/tsguide.html#naming-rules-by-identifier-type
also, i don't think you really need the strings fwiw, there may be a few rare cases where the value itself gets used for some javascript/api logic but if it's used primarily as a categorization method in typescript-only code, i think we can do something like this?
same thing for the others!