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

Input spec #20035

Merged
merged 4 commits into from
Nov 2, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
106 changes: 59 additions & 47 deletions packages/react-input/Spec-styling.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,45 +11,39 @@ General abbreviations used:
## Sizes

- padding and gap values are from (theoretical) `spacing.horizontal` unless otherwise specified
- bookend-related sizes are from separate bookends page (everything except L/R padding and spacing within inherits from default)
Copy link
Member Author

Choose a reason for hiding this comment

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

I updated this file to remove the parts about bookends and update naming, but it's mostly just notes for implementation purposes rather than actually being part of the formal spec, so less important to review in detail.

- interpretation:
- "spacing between icon before and content"/"spacing between content and icon after 1" => "spacing start/end to content"
- "spacing between icon after 1 and icon after 2" => "spacing within insideEnd" because we're not going to have two icon slots
- bookend "spacing between content and icon" => "spacing within bookend"
- "spacing between icon after 1 and icon after 2" => "spacing within contentBefore/After" because we're not going to have two icon slots
- omitted "focus indicator" b/c that's handled elsewhere

| Style | All |
| ------------- | ------------------- |
| v-align | vertically centered |
| border radius | medium |

| Style | medium | small | large |
| ----------------------------- | ---------------- | ------------------- | --------- |
| height | 32px | 24px | 40px |
| left/right padding | mNudge | sNudge | m |
| left/right padding in content | xxs | " | sNudge |
| bookends left/right padding | s | sNudge | m |
| content size | body1 (base.300) | caption1 (base.200) | base.400 |
| "icon" size | 20Regular | 16Regular | 24Regular |
| spacing start/end to content | xxs | " | sNudge |
| spacing within insideEnd | xs | " | " |
| spacing within bookend | xs | " | " |
| Style | medium | small | large |
| ---------------------------------- | ---------------- | ------------------- | --------- |
| height | 32px | 24px | 40px |
| left/right padding | mNudge | sNudge | m |
| left/right padding in content | xxs | " | sNudge |
| content size | body1 (base.300) | caption1 (base.200) | base.400 |
| "icon" size | 20Regular | 16Regular | 24Regular |
| spacing start/end to content | xxs | " | sNudge |
| spacing within contentBefore/After | xs | " | " |

### Sizes application

| Style | Slot | Notes |
| ----------------------------- | ---------------------------- | ---------------------------------------------------------------- |
| v-align | root, inputWrapper | ??? |
| height | root | ? as minHeight or height ? |
| border radius | bookends, inputWrapper, root | set where borders or shadow are defined; don't use if underlined |
| left/right padding | inputWrapper | padding |
| left/right padding in content | input | padding |
| bookends left/right padding | bookends | padding |
| content size | root, input | fontSize; doesn't inherit to input |
| "icon" size | n/a | no icons built in |
| spacing start/end to content | inputWrapper | display: flex (also to grow input), flex gap |
| spacing within insideEnd | insideEnd | display: flex, flex gap |
| spacing within bookends | bookends | display: flex, flex gap |
| Style | Slot | Notes |
| ---------------------------------- | ------------------- | ---------------------------------------------------------------- |
| v-align | root | ??? |
| height | root | ? as minHeight or height ? |
| border radius | root | set where borders or shadow are defined; don't use if underlined |
| left/right padding | root | padding |
| left/right padding in content | input | padding |
| content size | root, input | fontSize; doesn't inherit to input |
| "icon" size | n/a | no icons built in |
| spacing start/end to content | root | display: flex (also to grow input), flex gap |
| spacing within contentBefore/After | contentBefore/After | display: flex, flex gap |

## Appearance colors and strokes

Expand Down Expand Up @@ -86,26 +80,39 @@ General abbreviations used:

### Appearance application

| Style | Slot | Notes |
| -------------------------- | ------------------- | -------------------------------------------------------- |
| content color | input | other things have their own colors |
| placeholder color | input | `::placeholder` |
| "icon" color | insideStart/End | |
| shadow | root | encompasses bookends; requires rounding root corners |
| background | inputWrapper, input | bookends have separate background; input doesn't inherit |
| border | inputWrapper | |
| border hover | TODO inputWrapper | `:hover` |
| border pressed | TODO | |
| border focused | TODO inputWrapper | `:focus-within` |
| borderBottom | inputWrapper | |
| borderBottom hover | TODO inputWrapper | `:hover` |
| borderBottom pressed | TODO | |
| borderBottom focused | n/a | handled by focus indicator |
| in focus indicator | TODO | |
| in focus indicator pressed | TODO | |
| cursor | root, input | |

## Bookend appearance (TODO)
| Style | Slot | Notes |
| -------------------------- | ------------------- | ---------------------------------- |
| content color | input | other things have their own colors |
| placeholder color | input | `::placeholder` |
| "icon" color | contentBefore/After | |
| shadow | root | |
| background | root, input | |
| border | root | |
| border hover | TODO root | `:hover` |
| border pressed | TODO | |
| border focused | TODO root | `:focus-within` |
| borderBottom | root | |
| borderBottom hover | TODO root | `:hover` |
| borderBottom pressed | TODO | |
| borderBottom focused | n/a | handled by focus indicator |
| in focus indicator | TODO | |
| in focus indicator pressed | TODO | |
| cursor | root, input | |

## Bookends (deferred)

Bookend implementation has been deferred and will likely be handled in a separate component, but leaving these for reference.

### Sizes

| Style | medium | small | large |
| ------------------ | ------------------- | ------ | ----- |
| v-align | vertically centered | " | " |
| border radius | medium | " | " |
| left/right padding | s | sNudge | m |
| spacing within | xs | " | " |

### Appearance

| Style | filled | brand | transparent |
| --------------- | ------------------ | ------------------------ | --------------------- |
Expand All @@ -117,3 +124,8 @@ General abbreviations used:
- Inner border ("border right") color is applied separately to before/after bookends.
- Others are applied in obvious way to both bookends.
- All borders are thin (1px).

### Changes to default input appearance

- Remove rounded corners from input
- For filled inputs with shadow, change the shadow to also encompass the bookends
99 changes: 99 additions & 0 deletions packages/react-input/Spec-variants.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
# Input Variants
Copy link
Member Author

Choose a reason for hiding this comment

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

Clarification: this is NOT the main spec and not intended to be 100% finished yet, it's just notes about tentative plans and some comparison with v8 since that's what I'm most familiar with. A detailed spec and full comparison for any of these variants will be filled out once we're closer to implementing them.


Input has many variants and additional features. We don't plan to implement any of these immediately, but adding notes here for future reference.

## Bookends

"Bookends" (the naming comes from the design spec) are bits of text on the left or right outside of the field, surrounded by a border or background color.

Initially, bookends were planned to be included in the main component (as `bookendBefore`/`bookendAfter` slots) but we decided they're not essential and should be split out to simplify the DOM structure and defer some of the related open questions until later.

If/when a partner needs this feature, it will likely go in some kind of wrapper component.

Visual variants for the bookends are as follows (note that specific colors are just examples based on the default theme):

- `filled` (default): bookends have a gray background, black text, and no border
- `brand`: bookends have a brand color (such as blue or purple) background, white text, and no border
- `transparent`: bookends have a transparent background, black text, and only a thin inside border to distinguish them from the field

## Multi-line (`textarea`)

This will tentatively be a separate component, likely sharing some internals with the basic `Input` using hooks (scheduling TBD).

| Prop/concept | v8 | v0 | Proposal |
| ------------------ | ---------------------------- | --- | -------------------------------------------------------- |
| resizable | `resizable?: boolean` | | via native props |
| auto-adjust height | `autoAdjustHeight?: boolean` | | TBD (common request but has nasty implementation issues) |

Similar to both v8 and v0, passing other `textarea` native props as top-level component props will be supported.

### Possibility: `useTextInput`

We might want to make a hook and some types to share props and behavior between `Input` and the potential future `TextArea` (which would depend on `react-input` but with some different props and behaviors).

```ts
type TextInputSlots<
TInput extends HTMLInputElement | HTMLTextAreaElement,
TInputAttributes = TInput extends HTMLInputElement
? React.InputHTMLAttributes<TInput>
: React.TextAreaHTMLAttributes<TInput>
> = {
root: ObjectShorthandProps<React.HTMLAttributes<HTMLElement>, HTMLElement>;
input: ObjectShorthandProps<TInputAttributes, TInput>;
};
```

## Validation

Validation features and error messages will not be built into the new `Input`. Possible approaches:

- Hooks and/or a `Field` component to provide visual consistency and proper accessibility behavior across different types of inputs.
- A `Form` component for more complex scenarios involving validation across multiple fields.

### In v8

```ts
// relevant props only
interface ITextFieldProps {
// static error message
errorMessage?: string | JSX.Element;
// called to determine whether input is valid and show error if not
// (on all changes by default; modified by validateOnFocusIn/Out)
onGetErrorMessage?: (value: string) => string | JSX.Element | PromiseLike<string | JSX.Element> | undefined;
// whether to validate when focus moves into input (NOT on change)
validateOnFocusIn?: boolean;
// whether to validate when focus moves out of input (NOT on change)
validateOnFocusOut?: boolean;
// whether to validate when input is initially rendered
validiateOnLoad?: boolean;
// wait to validate until user stops typing by this ms
deferredValidationTime?: number;
// called after validation completes
onNotifyValidationResult?: (errorMessage: string | JSX.Element, value: string | undefined) => void;
// ...
}
```

### In v0

TBD
ecraig12345 marked this conversation as resolved.
Show resolved Hide resolved

## Password

v8 supports a password field with a custom reveal password button:

```tsx
<TextField type="password" canRevealPassword revealPasswordAriaLabel="Show password" />
```

v0 does not appear to have a similar feature.

TBD if/how we want to handle this in converged.
khmakoto marked this conversation as resolved.
Show resolved Hide resolved

## Masking

"Masking" refers to a text input with some pre-specified format that gets filled in as the user types, like `(___) ___-____`.

In v8 it's handled by `MaskedTextField`.

Masking is tricky to implement properly and bad for accessibility, so we don't plan to implement it in converged unless there's a compelling product requirement. At that point we'll also evaluate whether there are any suitable 3rd-party libraries which could handle it.
ecraig12345 marked this conversation as resolved.
Show resolved Hide resolved
Loading