Skip to content
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

feat(card): initial card implementation #145

Merged
merged 31 commits into from Nov 14, 2019
Merged
Changes from 6 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
2595fb9
feat(card): initial card implementation
GiantRobots Oct 18, 2019
fb615d6
feat(card): new utilities
GiantRobots Oct 18, 2019
8ada1c1
feat(card): fixed box dependency and paste-type dependencies
GiantRobots Oct 18, 2019
0541dd9
feat(card): fix lint errors, make types a little nicer
GiantRobots Oct 21, 2019
8712f09
feat(card): dropped lint rules that weren't working right
GiantRobots Oct 21, 2019
cd91fbb
feat(card): added documentation portion of the card
GiantRobots Oct 22, 2019
684d3be
feat(card): add in body color
GiantRobots Oct 23, 2019
32e2f68
feat(card): fixed package.json
GiantRobots Oct 23, 2019
31a3fa9
feat(card): throw errors on bad attribute pass through
GiantRobots Nov 13, 2019
86afcbc
feat(card): drop a comment
GiantRobots Nov 13, 2019
278822f
feat(card): fixed prop error to allow data props through
GiantRobots Nov 13, 2019
4c627d9
feat(card): some fix
GiantRobots Nov 13, 2019
71ccd47
feat(card): fix snaps
GiantRobots Nov 13, 2019
de55726
feat(card): some fix
GiantRobots Nov 13, 2019
379ad39
Merge branch 'master' into Card
GiantRobots Nov 14, 2019
74a47cc
feat(card): some fix
GiantRobots Nov 13, 2019
c86235f
Merge remote-tracking branch 'origin/master'
GiantRobots Nov 14, 2019
553f7fc
Merge branch 'master' into Card
GiantRobots Nov 14, 2019
1a8d31c
feat(card): fix test verbiage
GiantRobots Nov 14, 2019
faf3dd1
feat(card): update snaps, fix linting errors
GiantRobots Nov 14, 2019
5fe8971
feat(card): resolve lint issues by moving files,
GiantRobots Nov 14, 2019
1fcc7c4
feat(card): move snapshots
GiantRobots Nov 14, 2019
113e879
feat(card): fix paste types dependency
GiantRobots Nov 14, 2019
06faf4c
feat(card): fix weird typeness
GiantRobots Nov 14, 2019
b8b5104
feat(card): drop id to allow build move @types react down to devdeps
GiantRobots Nov 14, 2019
b0ebdee
feat(card): update based on simon's recommendations
GiantRobots Nov 14, 2019
e17df9d
feat(card): drop border passsthrough
GiantRobots Nov 14, 2019
842ba24
feat(card): global html attrs
GiantRobots Nov 14, 2019
c729ab6
feat(card): no stale properties
GiantRobots Nov 14, 2019
e89c8df
feat(card): drop unused utilities, fixed eslint sort order
GiantRobots Nov 14, 2019
eba4d55
feat(card): update snaps
GiantRobots Nov 14, 2019
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -43,6 +43,7 @@
"dependencies": {
"@emotion/core": "^10.0.15",
"@emotion/styled": "^10.0.15",
"@types/react": "16.8.23",
This conversation was marked as resolved by GiantRobots

This comment has been minimized.

Copy link
@TheSisb

TheSisb Oct 23, 2019

Collaborator

Any reason why you moved this up here from devDeps?

This comment has been minimized.

Copy link
@GiantRobots

GiantRobots Nov 12, 2019

Author Contributor

I did it because using types as a utility and interface seems to be in line with you need this for development. Unless I have dependencies/devDependencies wrong.

This conversation was marked as resolved by GiantRobots

This comment has been minimized.

Copy link
@SiTaggart

SiTaggart Oct 23, 2019

Collaborator

what's the reasoning for making this an exact dependency over a non-locked dev dependency?

This comment has been minimized.

Copy link
@GiantRobots

GiantRobots Oct 23, 2019

Author Contributor

Solid point I think it was an oversight because I installed that as a specific dependency to match what was in devDependencies. I'll update.

"emotion": "^10.0.17",
"react": "^16.8.6",
"react-dom": "^16.8.6",
@@ -72,7 +73,6 @@
"@storybook/react": "^5.2.3",
"@types/enzyme": "^3.10.1",
"@types/jest": "^24.0.15",
"@types/react": "^16.8.23",
"@types/react-dom": "^16.8.4",
"@types/react-test-renderer": "^16.8.2",
"@types/storybook__addon-actions": "^3.4.3",
@@ -2,7 +2,7 @@
"name": "@twilio-paste/card",
"version": "0.0.2",
"category": "data display",
"status": "backlog",
"status": "alpha",
"private": true,
"description": "",
"author": "Twilio Inc.",
@@ -26,9 +26,13 @@
},
"peerDependencies": {
"react": ">= 16.0.0",
"react-dom": ">= 16.0.0"
"react-dom": ">= 16.0.0",
"@twilio-paste/box": "^0.3.1",
"@twilio-paste/types": "^0.0.1"
},
"devDependencies": {
"@twilio-paste/box": "^0.3.1",
"@twilio-paste/types": "^0.0.1",
"rollup": "^1.16.2",
"rollup-plugin-babel": "^4.3.3",
"rollup-plugin-commonjs": "^10.0.1",
@@ -1,25 +1,39 @@
import * as React from 'react';
import {Box} from '@twilio-paste/box';
import {SpacingProps} from '@twilio-paste/types';
import {StaticDiv} from '@twilio-paste/types/src/DomTypes';

interface CardTitleProps {
children: React.ReactNode;
}
const CardTitle: React.FunctionComponent<CardTitleProps> = ({children}) => <h4>{children}</h4>;
interface CardFooterProps extends SpacingProps, StaticDiv {}

interface CardBodyProps {
children: React.ReactNode;
}
const CardBody: React.FunctionComponent<CardBodyProps> = ({children}) => <div>{children}</div>;
const CardFooter: React.FunctionComponent<CardFooterProps> = ({children, ...attributes}) => (
<Box
borderTopWidth="borderWidth10"
borderBottomWidth="borderWidth0"
borderLeftWidth="borderWidth0"
borderRightWidth="borderWidth0"
marginTop="space40"
paddingTop="space40"
borderColor="colorBorder"
borderStyle="solid"
{...attributes}
>
{children}
</Box>
);

interface CardProps {
onClick: () => void;
children: React.ReactNode;
}
interface CardProps extends SpacingProps, StaticDiv {}

const Card: React.FunctionComponent<CardProps> = ({children, onClick}) => (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions, jsx-a11y/click-events-have-key-events, jsx-a11y/no-noninteractive-tabindex
<aside onClick={onClick} tabIndex={onClick != null ? 0 : undefined} role={onClick != null ? 'button' : undefined}>
const Card: React.FunctionComponent<CardProps> = ({children, ...attributes}) => (
<Box
borderWidth="borderWidth20"
borderColor="colorBorder"
borderStyle="solid"
borderRadius="borderRadius20"
padding="space60"
{...attributes}
This conversation was marked as resolved by GiantRobots

This comment has been minimized.

Copy link
@SiTaggart

SiTaggart Oct 23, 2019

Collaborator

Not a huge fan of spreading props like this because I can now change everything about the component outside of the systems control.

Our components should only be modifiable within the confines of the API that we define through variants. If you don't think the component works for you, you can drop down to the Box and Text layer and create your custom experience.

This comment has been minimized.

Copy link
@GiantRobots

GiantRobots Oct 23, 2019

Author Contributor

This is definitely a subject to talk about. The current implementation if you're using Typescript will only allow attributes that are defined and extended. This works great if you're using typescript...

If you're not using Typescript you can put anything in there and that sucks. +1

This comment has been minimized.

Copy link
@GiantRobots

GiantRobots Oct 23, 2019

Author Contributor

If we don't spread then we have to pass through all of the properties (all aria, all events, all etc) which will that hard to maintain Aria, and event passthrough.

This comment has been minimized.

Copy link
@GiantRobots

GiantRobots Oct 23, 2019

Author Contributor
if (procces.env.TypeChecking) {
    {...attributes}
} else {
  filteredAttributes = filterAttrs(attributes);
  {...filteredAttributes}
}

This comment has been minimized.

Copy link
@SiTaggart

SiTaggart Oct 23, 2019

Collaborator

Explain the above code snippet to me in more detail, please

This comment has been minimized.

Copy link
@GiantRobots

GiantRobots Oct 24, 2019

Author Contributor

So if a user opts into TypeChecking by setting up Typescript or fork-ts-checker-webpack-plugin to ensure they have valid typings they can also set an env flag at the startup of the application like...

new webpack.DefinePlugin({
  TypeChecking: JSON.stringify(true),
});

once a user has "opted in" to Typechecking then we can take the "easy path" we can use Types as a source of truth and drop a lot of the runtime checking and just passthrough attributes that match closer to the types that are coming from the React types package.

Another thought we should probably drop runtime checking when process.env.NODE_ENV === 'production' to mirror React's functionality and dropping development warnings when run in production mode

This comment has been minimized.

Copy link
@SiTaggart

SiTaggart Oct 30, 2019

Collaborator

OK, so given no one really has a good answer to solve this right now, we're making the decision not to solve it just yet, in favour of being explicit.

We as component authors should have a reasonable idea of what is appropriate to add to an element from semantic and accessibility point of view given the intended use case of the component/design. This should inform us to declare a set of props a consumer would likely need.

This will be plenty good enough to unblock us and get rolling.

>
{children}
</aside>
</Box>
);

export {Card, CardBody, CardTitle};
export {Card, CardFooter};
@@ -1,11 +1,35 @@
import * as React from 'react';
import {storiesOf} from '@storybook/react';
import {action} from '@storybook/addon-actions';
import {Card, CardTitle, CardBody} from '../src';
// import {action} from '@storybook/addon-actions';
This conversation was marked as resolved by GiantRobots

This comment has been minimized.

Copy link
@TheSisb

TheSisb Oct 23, 2019

Collaborator

Can just remove this line

import {Heading} from '@twilio-paste/heading';
import {Text} from '@twilio-paste/text';
import {Card, CardFooter} from '../src';

storiesOf('Components|Card', module).add('Default', () => (
<Card onClick={action('clicked')}>
<CardTitle>Title</CardTitle>
<CardBody>Body</CardBody>
</Card>
));
storiesOf('Components|Card', module)
.add('Default', () => (
<Card>
This conversation was marked as resolved by GiantRobots

This comment has been minimized.

Copy link
@SiTaggart

SiTaggart Nov 14, 2019

Collaborator

Would it be possible to hook up the first story to be able to modify the props with the knobs feature of storybook?

Kind of like https://github.com/twilio-labs/paste/blob/master/packages/paste-core/utilities/text/stories/index.stories.tsx#L68

<Heading as="h2">With a heading</Heading>
<Text>Body</Text>
<CardFooter>
<Text>I&apos;m | The | Footer</Text>
</CardFooter>
</Card>
))
.add('Padding', () => (
<Card padding="space200">
<Heading as="h2">With a heading</Heading>
<Text>Body</Text>
</Card>
))
.add('No Padding', () => (
<Card padding="space0">
<Heading as="h2">With a heading</Heading>
<Text>Body</Text>
</Card>
))
.add('Prop Passthrough', () => (
<Card aria-busy aria-atomic data-qahook="ZeCard!" id="12">
<Heading as="h2">With a heading</Heading>
<Text>Body</Text>
</Card>
));
@@ -7,5 +7,12 @@
"include": [
"src/**/*"
],
"references": []
"references": [
{
"path": "../../utilities/box"
},
{
"path": "../../../paste-types"
}
]
}
@@ -28,7 +28,7 @@
},
"peerDependencies": {
"@emotion/styled": "^10.0.10",
"@twilio-paste/box": "^0.2.4",
"@twilio-paste/box": "^0.3.1",
"react": "^16.8.6",
"react-dom": "^16.8.6",
"styled-system": "^4.1.0"
This conversation was marked as resolved by GiantRobots

This comment has been minimized.

Copy link
@TheSisb

TheSisb Oct 23, 2019

Collaborator

We merged the styled-system v5 upgrade PR, can you make sure all these "styled-system" deps are on version 5.1.2 please? #18

This comment has been minimized.

Copy link
@GiantRobots

GiantRobots Nov 13, 2019

Author Contributor

Any guidance on how to do this merge better? I'd be happy to rebase or anything else.

@@ -0,0 +1,26 @@
import {DOMAttributes} from 'react';
This conversation was marked as resolved by GiantRobots

This comment has been minimized.

Copy link
@TheSisb

TheSisb Oct 23, 2019

Collaborator

We should add an export * from './DomTypes' in the index file so we can import these from the package directly

/* eslint-disable @typescript-eslint/no-empty-interface */
// This rule complains when we omit even though our type is not equivalent to the supertype

// Typescript consumers will get this no className passthrough but not js consumers
// Might want to come up with some way to prevent the injection of these types with js
type AttributesToDrop = 'className' | 'style';

/* eslint-disable-next-line @typescript-eslint/no-empty-interface */
export interface Events extends DOMAttributes<'div'> {}

// Unfortunately we can't extend BaseCompoment for these types because Omit
export interface StaticDiv extends Omit<InteractiveDiv, keyof Events> {}
This conversation was marked as resolved by GiantRobots

This comment has been minimized.

Copy link
@TheSisb

TheSisb Oct 23, 2019

Collaborator

nit: can we move these to after line 24 so that InteractiveDiv is defined before it's used please

export interface StaticLabel extends Omit<InteractiveLabel, keyof Events> {}

// Not a fan of having static interactiveX wish we could do
// interactive<T> = React.ComponentProps<T>; but couldn't make it work ¯\_(ツ)_/¯
export interface InteractiveDiv extends Omit<React.ComponentProps<'div'>, AttributesToDrop> {}
export interface InteractiveButton extends Omit<React.ComponentProps<'button'>, AttributesToDrop> {}
export interface InteractiveLabel extends Omit<React.ComponentProps<'label'>, AttributesToDrop> {}
export interface InteractiveAnchor extends Omit<React.ComponentProps<'a'>, AttributesToDrop> {}
export interface InteractiveInput extends Omit<React.ComponentProps<'input'>, AttributesToDrop> {}
export interface InteractiveImage extends Omit<React.ComponentProps<'img'>, AttributesToDrop> {}
export interface InteractiveSelect extends Omit<React.ComponentProps<'select'>, AttributesToDrop> {}
This conversation was marked as resolved by GiantRobots

This comment has been minimized.

Copy link
@TheSisb

TheSisb Oct 23, 2019

Collaborator

These are great! 👍


/* eslint-enable @typescript-eslint/no-empty-interface */
@@ -0,0 +1,160 @@
---
title: Card
package: '@twilio-paste/card'
description: The Twilio Paste Card
---

import {graphql} from 'gatsby';
import {Box} from '@twilio-paste/box';
import {Button} from '@twilio-paste/button';
import {Card, CardFooter} from '@twilio-paste/card';
import {Heading} from '@twilio-paste/heading';
import {Text} from '@twilio-paste/text';
import Changelog from '@twilio-paste/card/CHANGELOG.md';
import {DoDont, Do, Dont} from '../../../components/DoDont';
import {Callout, CalloutTitle, CalloutText} from '../../../components/callout';
import {SidebarCategoryRoutes} from '../../../constants';

export const pageQuery = graphql`
{
allPasteComponent(filter: {name: {eq: "@twilio-paste/card"}}) {
edges {
node {
name
description
status
version
}
}
}
mdx(fields: { slug: { eq: "/card/" } }) {
headings {
depth
value
}
}
}
`;
<ComponentHeader
name="Card"
categoryRoute={SidebarCategoryRoutes.COMPONENTS}
githubUrl="https://github.com/twilio-labs/paste/blob/master/packages/paste-core/components/card/"
storybookUrl="https://twilio-labs.github.io/paste/?path=/story/components-card--default"
data={props.data.allPasteComponent.edges}
/>
---
<contentwrapper>
<TableOfContents headings={props.data.mdx.headings} />
<content>
## Guidelines
### About Cards
Cards are a specifically-styled container that group relatable content and actions.
Card is an extremely flexible container that does not require specific components inside of it,
allowing an implementer to compose as needed, but elements such as Title, Body,
and Button or Anchor are commonly used. See Examples for common Card patterns.
A Card does not currently handle any interactive events such as hover, click or focus,
though children composed inside of it may commonly have event handlers.
Acknowledging the flexibility of Card component, there are several non-negotiable
properties of a Card that differentiate it from a more basic page-layout element, such as Box:
* A background color of $background-color-body.
This conversation was marked as resolved by GiantRobots

This comment has been minimized.

Copy link
@SiTaggart

SiTaggart Nov 14, 2019

Collaborator

This token is called $color-background-body. Might want to double check the others too if this slipped through spec review.

* A border width of $border-width-20.
* A border radius of $border-radius-20.
* A border color of $color-border.
* A flexible width and height.
* Height is determined by a Card’s child components
* Width is determined by the parent component or container surrounding Card
### Card vs. Box
A card is a box with default attributes defined above. The purpose of the card is to provide a consistent experience to use across the website the design team can
## Usage Guide
### Installation
```bash
yarn add @twilio-paste/card
or
npm i @twilio-paste/card
```

### API

| Prop | Type | Description | Default |
|-------------- |------------------ |------------------------------------------ |---------------- |
| children? | React.ReactNode | Child components to render into the card | undefined |
| borderWidth | borderWidths | Width of the border | borderWidth20 |
| borderColor | BorderColors | Color of the border | colorBorder |
| borderStyle | BorderStyleProps | Style of the border | solid |
| borderRadius | Radii | Radius of the border | borderRadius20 |
| padding | Spacing | Internal spacing of the card | space60 |
This conversation was marked as resolved by GiantRobots

This comment has been minimized.

Copy link
@SiTaggart

SiTaggart Nov 14, 2019

Collaborator

This prop table is out of date a little bit, you only allow "Global HTML attributes" and PaddingProps now


### Examples
This conversation was marked as resolved by GiantRobots

This comment has been minimized.

Copy link
@SiTaggart

SiTaggart Nov 14, 2019

Collaborator

We usually place examples before the Usage Guidelines.


<LivePreview scope={{Card, Text, CardFooter, Heading}} language="jsx">
{`
<Card>
<Heading as="h2">With a heading</Heading>
<Text>Body</Text>
<CardFooter>
<Text>I&apos;m | The | Footer</Text>
</CardFooter>
</Card>
`}
</LivePreview>
<LivePreview scope={{Card, Text, CardFooter, Heading, Button}} language="jsx">
{`
<Card>
<Heading as="h2">This is a card with buttons!</Heading>
<Text>
Lorem ipsum dolor sit amet, consectetur adipiscing elit. Nullam dui tellus,
tristique in tincidunt cursus, efficitur at felis. Phasellus imperdiet,
lorem et commodo egestas, arcu.
</Text>
<CardFooter>
<Button variant="destructive">Cancel</Button>
<Button variant="submit">Submit</Button>
</CardFooter>
</Card>
`}
</LivePreview>
<LivePreview scope={{Card, Text, CardFooter, Heading}} language="jsx">
{`
<Card padding="space200">
<Heading as="h2">With some padding</Heading>
<Text>Body</Text>
</Card>
`}
</LivePreview>
<LivePreview scope={{Card, Text, CardFooter, Heading}} language="jsx">
{`
<Card aria-busy aria-atomic data-qahook="ZeCard!" id="12">
<Heading as="h2">Prop Passthrough</Heading>
<Text>Body</Text>
</Card>
`}
</LivePreview>
</content>
</contentwrapper>
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.