Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

Add inputAs prop to TextField to customize how the underlying input is rendered #72

Merged
merged 3 commits into from
Aug 22, 2019
Merged

Add inputAs prop to TextField to customize how the underlying input is rendered #72

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f6d5d77
feat(TextField): add ability to customize `<input>` element with `inp…
justinanastos Aug 19, 2019
8e92e9b
docs(TextField): Add story to show how to use `inputAs`
justinanastos Aug 19, 2019
e4de344
feat(TextField): add `onBlur` prop
justinanastos Aug 19, 2019
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
26 changes: 25 additions & 1 deletion src/TextField/TextField.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/** @jsx jsx */
import { jsx } from "@emotion/core";
import { jsx, ClassNames } from "@emotion/core";
import { storiesOf } from "@storybook/react";
import { TextField } from "./TextField";
import { DemoSection } from "../shared/DemoSection";
Expand Down Expand Up @@ -87,6 +87,30 @@ storiesOf("TextField", module).add("Catalog", () => (
/>
</VerticalTextFieldGroup>
</DemoSection>
<DemoSection>
<VerticalTextFieldGroup
title="Custom Input via inputAs"
description="You can use the `inputAs` prop to customize what elmement or how you render the underlying input component"
>
<ClassNames>
{({ cx, css }) => (
<TextField
inputAs={
justinanastos marked this conversation as resolved.
Show resolved Hide resolved
<input
className={cx(
css({
"&, :focus, :hover": { border: "1px solid blue" },
})
)}
/>
}
placeholder="Placeholder text"
label="Blue border input"
/>
)}
</ClassNames>
</VerticalTextFieldGroup>
</DemoSection>
<DemoSection>
<VerticalTextFieldGroup
title="Input States"
Expand Down
272 changes: 151 additions & 121 deletions src/TextField/TextField.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
/** @jsx jsx */
import { jsx } from "@emotion/core";
import { ClassNames, jsx } from "@emotion/core";
import React from "react";
import * as typography from "../typography";
import { colors } from "../colors";
import { IconAlertSolid } from "../icons/IconAlertSolid";
import { IconInfoSolid } from "../icons/IconInfoSolid";
import classnames from "classnames";

interface Props {
/**
Expand Down Expand Up @@ -44,6 +45,16 @@ interface Props {
*/
icon?: React.ReactNode;

/**
* Override how the `input` is rendered. You can pass either an intrinisic jsx element as a string (like "input") or a react element (`<input />`)
*
* If you pass a react element, props that we add are spread onto the input.
*
* @default "input"
*/
inputAs?: React.ReactElement | keyof JSX.IntrinsicElements;

onBlur?: (event: React.ChangeEvent<HTMLInputElement>) => void;
onChange?: (event: React.ChangeEvent<HTMLInputElement>) => void;

/**
Expand Down Expand Up @@ -94,149 +105,168 @@ export const TextField: React.FC<Props> = ({
className,
defaultValue,
description,
inputAs = "input",
disabled,
error,
helper,
icon,
label,
onBlur,
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why this PR touches onBlur and what otherProps used to do?

Copy link
Contributor Author

@justinanastos justinanastos Aug 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I tried to sneak this in because I thought it would go through fast. I'm going to pull it into another patch PR. I explicitly don't want to accept otherProps because, this being a layout component, it's not obvious where those props will be spread to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I remember why I did it this way; these two work together because there are formatting changes that will cause merge conflicts and I wanted to avoid that.

onChange,
placeholder,
showInfoIcon,
size = "standard",
value,
...otherProps
}) => {
return (
<div className={className}>
<label
css={{
paddingBottom: 8,
...typography.base.base,
fontWeight: 600,
}}
>
{label != null && <div css={{ marginBottom: 4 }}>{label}</div>}
{description != null && (
<div
css={{
...typography.base.base,
color: colors.black.base,
}}
}) => (
<ClassNames>
{({ css, cx }) => {
const inputProps = {
defaultValue,
disabled,
onBlur,
onChange,
placeholder,
value,
className: cx(
css({
backgroundColor: disabled ? colors.silver.light : colors.white,
border: "solid 1px",
borderColor: error ? colors.red.base : colors.silver.darker,
"::placeholder": {
color: disabled ? colors.grey.lighter : colors.grey.light,
opacity: 1,
},
borderRadius: 4,
flex: 1,
height: size === "standard" ? 36 : size === "small" ? 28 : 42,
...(size === "small"
? typography.base.small
: typography.base.base),
marginRight: icon ? -30 : 0,
paddingLeft: icon ? 34 : size === "small" ? 8 : 12,
paddingRight: size === "small" ? 8 : 12,
width: "100%",
":hover, &[data-force-hover-state]": {
borderColor:
!disabled && !error
? colors.grey.light
: error
? colors.red.base
: colors.silver.darker,
},
":focus, &[data-force-focus-state]": {
borderColor:
!disabled && !error
? colors.blue.light
: error
? colors.red.base
: colors.silver.darker,
outline: "none",
},
})
),
};

return (
<div className={className}>
<label
className={cx(
css({
paddingBottom: 8,
...typography.base.base,
fontWeight: 600,
})
)}
>
{description}
</div>
)}
<div
css={{
marginTop: 8,
position: "relative",
}}
>
{icon && (
{label != null && <div css={{ marginBottom: 4 }}>{label}</div>}
{description != null && (
<div
css={{
...typography.base.base,
color: colors.black.base,
}}
>
{description}
</div>
)}
<div
css={{
position: "absolute",
display: "inline-flex",
left: 12,
top: "50%",
transform: "translateY(-50%)",
marginTop: 8,
position: "relative",
}}
>
{icon}
{icon && (
<div
css={{
position: "absolute",
display: "inline-flex",
left: 12,
top: "50%",
transform: "translateY(-50%)",
}}
>
{icon}
</div>
)}

{React.isValidElement(inputAs)
? React.cloneElement(inputAs, {
Copy link
Contributor

Choose a reason for hiding this comment

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

I read the api documentation for cloneElement and createElement to understand what is happening here. From what I understand, cloneElement does a shallow copy of the props by default, so we need to merge the classNames explicitly. Will this have unintended consequences or semantics around class precedence that would require documentation?

The other case uses createElement without merging class names, since there are no props for inputAs when it is a string, such as div or input. It took me about 10 minutes to read through the React docs and understand it. I suggest you add a comment describing the cases here and why the prop merging is different, so hopefully that we don't need to context switch out of the source to understand the behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will this have unintended consequences or semantics around class precedence that would require documentation

Great question. No, the order of the class names in class="a b c" has no effect on precedence. Precedence is determined solely by specificity, which has clearly defined rules in the css spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest you add a comment describing the cases here and why the prop merging is different, so hopefully that we don't need to context switch out of the source to understand the behavior

I very much value the suggestion and am happy that you read through the docs before bringing up the talking point.

First, understanding the difference between an element and component is critical. A component is the definition of something you will render, either a React.FC (functional component) or a class that extends React.Component. An element is the result of a render by either of two methods (note these two are identical; the first will be transpiled into the second by our build tools):

  • jsx

    <a href="apollographql.com">link</a>

  • React.createElement

    React.createElement('a', {href: 'apollographql.com'}, 'link')

Note: jsx is transpiled into React.createElement for use in browsers.

As you know, createElement and cloneElement are very different things. cloneElement takes an element, meaning something you've already rendered, and augments it with additional props; hence the classNames merging. createElement takes either an intrinsic jsx element (like "div") or a component and renders it into a react element.

While this not trivial, I think it's more valuable to look at the React docs if someone wants to know how it works rather than explaining it in a comment. In my opinion, I value that you spent the time to read about createElement and cloneElement and I think you got far more out of that than any comment I could leave here. I also think it's better to read the docs because then in the next component where we do the same thing (Buttons, for example), you already know how they all work.

...inputProps,
className: classnames(
inputProps.className,
inputAs.props.className
),
})
: React.createElement(inputAs, inputProps)}
</div>
)}

<input
{...otherProps}
defaultValue={defaultValue}
disabled={disabled}
onChange={onChange}
placeholder={placeholder}
value={value}
css={{
backgroundColor: disabled ? colors.silver.light : colors.white,
border: "solid 1px",
borderColor: error ? colors.red.base : colors.silver.darker,
"::placeholder": {
color: disabled ? colors.grey.lighter : colors.grey.light,
opacity: 1,
},
borderRadius: 4,
flex: 1,
height: size === "standard" ? 36 : size === "small" ? 28 : 42,
...(size === "small"
? typography.base.small
: typography.base.base),
marginRight: icon ? -30 : 0,
paddingLeft: icon ? 34 : size === "small" ? 8 : 12,
paddingRight: size === "small" ? 8 : 12,
width: "100%",
":hover, &[data-force-hover-state]": {
borderColor:
!disabled && !error
? colors.grey.light
: error
? colors.red.base
: colors.silver.darker,
},
":focus, &[data-force-focus-state]": {
borderColor:
!disabled && !error
? colors.blue.light
: error
? colors.red.base
: colors.silver.darker,
outline: "none",
},
}}
/>
</div>
</label>
<div
css={{
marginTop: 8,
alignItems: "center",
position: "relative",
}}
>
{(helper || error) && (
</label>
<div
css={{
...typography.base.small,
color: error ? colors.red.base : colors.grey.base,
display: "flex",
marginRight: 8,
marginTop: 8,
paddingLeft: size === "small" ? 8 : 12,
alignItems: "center",
position: "relative",
}}
>
{error ? (
<IconAlertSolid
{(helper || error) && (
<div
css={{
height: 15,
...typography.base.small,
color: error ? colors.red.base : colors.grey.base,
display: "flex",
marginRight: 8,
position: "relative",
top: 2,
width: 15,
marginTop: 8,
paddingLeft: size === "small" ? 8 : 12,
}}
/>
) : showInfoIcon && helper ? (
<IconInfoSolid
css={{
color: colors.blue.base,
height: 15,
marginRight: 8,
position: "relative",
top: 2,
width: 15,
}}
/>
) : null}
>
{error ? (
<IconAlertSolid
css={{
height: 15,
marginRight: 8,
position: "relative",
top: 2,
width: 15,
}}
/>
) : showInfoIcon && helper ? (
<IconInfoSolid
css={{
color: colors.blue.base,
height: 15,
marginRight: 8,
position: "relative",
top: 2,
width: 15,
}}
/>
) : null}

<div>{error || helper}</div>
<div>{error || helper}</div>
</div>
)}
</div>
)}
</div>
</div>
);
};
</div>
);
}}
</ClassNames>
);