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: add inline-edit package #938

Merged
merged 26 commits into from
Jul 26, 2023
Merged
Show file tree
Hide file tree
Changes from 11 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
1 change: 1 addition & 0 deletions .storybook/styles.css
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ h1,
h2,
h3 {
color: var(--lp-color-text-ui-primary-base);
margin: 0;
}

@font-face {
Expand Down
1 change: 1 addition & 0 deletions apps/remix/app/data.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export async function getComponents() {
{ to: 'components/icon', name: 'Icon' },
{ to: 'components/icon-button', name: 'IconButton' },
{ to: 'components/inline', name: 'Inline' },
{ to: 'components/inline-edit', name: 'InlineEdit' },
{ to: 'components/markdown', name: 'Markdown' },
{ to: 'components/menu', name: 'Menu' },
{ to: 'components/modal', name: 'Modal' },
Expand Down
11 changes: 11 additions & 0 deletions apps/remix/app/routes/components.inline-edit.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { InlineEdit } from '@launchpad-ui/core';
import { useState } from 'react';

export default function Index() {
const [editValue, setEditValue] = useState('edit me');
return (
<InlineEdit defaultValue={editValue} onSave={setEditValue}>
<span>{editValue}</span>
</InlineEdit>
);
}
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
"@typescript-eslint/eslint-plugin": "^6.1.0",
"@typescript-eslint/parser": "^6.1.0",
"@vanilla-extract/css": "^1.12.0",
"@vanilla-extract/recipes": "^0.4.0",
"@vanilla-extract/vite-plugin": "^3.8.2",
"@vitejs/plugin-react-swc": "^3.3.1",
"@vitest/coverage-v8": "^0.33.0",
Expand Down
1 change: 1 addition & 0 deletions packages/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
"@launchpad-ui/focus-trap": "workspace:~",
"@launchpad-ui/form": "workspace:~",
"@launchpad-ui/inline": "workspace:~",
"@launchpad-ui/inline-edit": "workspace:~",
"@launchpad-ui/markdown": "workspace:~",
"@launchpad-ui/menu": "workspace:~",
"@launchpad-ui/modal": "workspace:~",
Expand Down
2 changes: 2 additions & 0 deletions packages/core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ export type { StackProps } from '@launchpad-ui/stack';
export type { Space } from '@launchpad-ui/types';
export type { InlineProps } from '@launchpad-ui/inline';
export type { ColumnProps, ColumnsProps } from '@launchpad-ui/columns';
export type { InlineEditProps } from '@launchpad-ui/inline-edit';
// plop end type exports

// plop start module exports
Expand Down Expand Up @@ -194,4 +195,5 @@ export { Tooltip, TooltipBase } from '@launchpad-ui/tooltip';
export { Stack } from '@launchpad-ui/stack';
export { Inline } from '@launchpad-ui/inline';
export { Column, Columns } from '@launchpad-ui/columns';
export { InlineEdit } from '@launchpad-ui/inline-edit';
// plop end module exports
14 changes: 14 additions & 0 deletions packages/inline-edit/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# @launchpad-ui/inline-edit

> An element used to display and allow inline editing of a form element value.

[![See it on NPM!](https://img.shields.io/npm/v/@launchpad-ui/inline-edit?style=for-the-badge)](https://www.npmjs.com/package/@launchpad-ui/inline-edit)
[![How big is this package in your project?](https://img.shields.io/bundlephobia/minzip/@launchpad-ui/inline-edit?style=for-the-badge)](https://bundlephobia.com/result?p=@launchpad-ui/inline-edit)

## Installation

```sh
$ yarn add @launchpad-ui/inline-edit
# or
$ npm install @launchpad-ui/inline-edit
```
25 changes: 25 additions & 0 deletions packages/inline-edit/__tests__/InlineEdit.cy.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { InlineEdit } from '../src';

describe('InlineEdit', () => {
it('renders', () => {
const editValue = 'test';
cy.mount(
<InlineEdit defaultValue={editValue} onSave={() => undefined}>
<span>{editValue}</span>
</InlineEdit>
);
cy.getByTestId('inline-edit').should('be.visible');
});

it('is accessible', () => {
const editValue = 'test';
cy.mount(
<InlineEdit defaultValue={editValue} onSave={() => undefined}>
<span>{editValue}</span>
</InlineEdit>
);
cy.checkA11y();
cy.getByRole('button').click();
cy.checkA11y();
});
});
24 changes: 24 additions & 0 deletions packages/inline-edit/__tests__/InlineEdit.spec.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import type { InlineEditProps } from '../src';

import { useState } from 'react';
import { it, expect, describe } from 'vitest';

import { render, screen } from '../../../test/utils';
import { InlineEdit } from '../src';

const InlineEditComponent = ({ ...props }: Partial<InlineEditProps>) => {
const [editValue, setEditValue] = useState('test');

return (
<InlineEdit defaultValue={editValue} {...props} onSave={setEditValue}>
<span>{editValue}</span>
</InlineEdit>
);
};

describe('InlineEdit', () => {
it('renders', () => {
render(<InlineEditComponent />);
expect(screen.getByTestId('inline-edit')).toBeInTheDocument();
});
});
50 changes: 50 additions & 0 deletions packages/inline-edit/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
{
"name": "@launchpad-ui/inline-edit",
"version": "0.0.1",
"status": "alpha",
"publishConfig": {
"access": "public"
},
"description": "An element used to display and allow inline editing of a form element value.",
"files": [
"dist"
],
"main": "dist/index.js",
"module": "dist/index.es.js",
"types": "dist/index.d.ts",
"sideEffects": [
"**/*.css"
],
"exports": {
".": {
"types": "./dist/index.d.ts",
"import": "./dist/index.es.js",
"require": "./dist/index.js"
},
"./package.json": "./package.json",
"./style.css": "./dist/style.css"
},
"source": "src/index.ts",
"scripts": {
"build": "vite build -c ../../vite.config.ts && tsc --project tsconfig.build.json",
"clean": "rm -rf dist",
"lint": "eslint '**/*.{ts,tsx,js}' && stylelint '**/*.css' --ignore-path ../../.stylelintignore",
"test": "vitest run --coverage"
},
"dependencies": {
"@react-aria/button": "3.8.0",
"@react-aria/focus": "3.13.0",
"@react-aria/utils": "3.18.0",
"@launchpad-ui/tokens": "workspace:~",
"@launchpad-ui/vars": "workspace:~",
"classix": "2.1.17"
},
"peerDependencies": {
"react": "18.2.0",
"react-dom": "18.2.0"
},
"devDependencies": {
"react": "18.2.0",
"react-dom": "18.2.0"
}
}
137 changes: 137 additions & 0 deletions packages/inline-edit/src/InlineEdit.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
import type { InlineVariants } from './styles/InlineEdit.css';
import type { TextAreaProps, TextFieldProps } from '@launchpad-ui/form';
import type {
ComponentProps,
Dispatch,
KeyboardEventHandler,
ReactElement,
SetStateAction,
} from 'react';

import { ButtonGroup, IconButton } from '@launchpad-ui/button';
import { TextField } from '@launchpad-ui/form';
import { Icon } from '@launchpad-ui/icons';
import { useButton } from '@react-aria/button';
import { focusSafely } from '@react-aria/focus';
import { mergeProps, useUpdateEffect } from '@react-aria/utils';
import { cx } from 'classix';
import { cloneElement, useRef, useState } from 'react';

import { container, cancelButton, inline, readButton } from './styles/InlineEdit.css';

type InlineEditProps = ComponentProps<'div'> &
InlineVariants &
Pick<ComponentProps<'input'>, 'defaultValue'> & {
'data-test-id'?: string;
onSave: Dispatch<SetStateAction<string>>;
hideEdit?: boolean;
input?: ReactElement<TextFieldProps | TextAreaProps>;
};

const InlineEdit = ({
'data-test-id': testId = 'inline-edit',
layout = 'horizontal',
children,
defaultValue,
onSave,
hideEdit = false,
input = <TextField />,
'aria-label': ariaLabel,
}: InlineEditProps) => {
const [isEditing, setEditing] = useState(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question the one about defaultValue and un/controlled.

Are there use cases where it would be useful to pass isEditing from the owner component?

const inputRef = useRef<HTMLInputElement>(null);
const editRef = useRef<HTMLButtonElement>(null);

useUpdateEffect(() => {
isEditing
? inputRef.current && focusSafely(inputRef.current)
: editRef.current && focusSafely(editRef.current);
}, [isEditing]);

const handleEdit = () => {
setEditing(true);
};

const handleCancel = () => {
setEditing(false);
};

const handleSave = () => {
onSave(inputRef.current?.value || '');
setEditing(false);
};

const handleKeyDown: KeyboardEventHandler<HTMLInputElement> = (event) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Was exploring the new stories in this branch locally, noticed that pressing Esc will cancel an in-progress edit which brought me here, wondering if we should also consider the case where a user blurs away from the input/cancel/save buttons while editing (either by clicking outside of the affected elements, or via keyboard navigation)?

There is some nuance to consider, but I'm wondering about the case where multiple inline-editable fields exist next to each other... how would a user navigate between them? For example: if I begin editing a field but then try to keyboard navigate to another editable field, what should happen? I'd expect the edit for the first field to be implicitly cancelled, but curious to hear your thoughts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah no strong opinion on blur implicitly cancelling. Can try that out and see how that feels.

Copy link
Contributor

Choose a reason for hiding this comment

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

What are the downsides of leaving the inline edits in editing mode when tabbing around?

The escape has less ambiguous intent than tabbing around.

One way to think about this maybe: do we want this component to be modal, where interacting with another component would affect the state of this component? (Think popover: once it's open, if the user interacts with anything else on the page, it should close.)

Copy link
Contributor

@kwatkins-ld kwatkins-ld Jul 26, 2023

Choose a reason for hiding this comment

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

One question is predictability: by making the interaction modal we can create the affordance that the edit is only persisted if the user explicitly chooses to do so (by clicking the save button, or pressing Enter)... anything else causes the draft to be discarded. This question also has implications for when we might consider a form "dirty" (which overlaps with routing behavior).

In the absence of a modal interaction, it's hard to reason about why we'd choose inline edit at all... if a field that I begin editing can be left in the editing state to move on to another field, what value is inline-edit adding over plain inputs?

Also to be clear I think this is very much a question that requires design feedback, I had just started to think about this while implementing elsewhere so the questions are fresh in mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

what value is inline-edit adding over plain inputs?

Yeah. There are places where we use an inline edit to make a field opt-in, like flag keys upon creation.

This question also has implications for when we might consider a form "dirty" (which overlaps with routing behavior).

How does this overlap with routing?

If the user hasn't confirmed their input, I'd expect form's state to be unchanged.

Anyways, these are good points for UX crit. I don't think these are that complicated, so long as we're consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

How does this overlap with routing?

Today we consider a form dirty if the input hasn't been saved, and dirty forms (in most places) cause route changes to be blocked and the user is forced to confirm the navigation... if we make this interaction modal, blurring a field that is being edited would discard the draft (and therefore not cause the field to be considered dirty).

Conversely, if this interaction isn't modal the following scenario seems possible: begin editing a field, change the field's value, then navigate to a different field... in that case the first field would need to be considered dirty to prevent unexpectedly losing edits.

I also agree that these questions aren't especially complicated, and that we don't necessarily have to answer them in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

That said, we should be opinionated here before we show up at UX crit.

My 2¢: Let's cancel on blur for now. That feels snappier to me. 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Meeting with Antonio shortly and can go over this topic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed with Antonio and we are gonna go with cancel on blur 👍

if (event.key === 'Enter') {
event.preventDefault();
handleSave();
} else if (event.key === 'Escape') {
event.preventDefault();
handleCancel();
}
};

const { buttonProps } = useButton(
{
'aria-label': 'edit',
elementType: 'div',
onPress: handleEdit,
},
editRef
);

const renderReadContent = hideEdit ? (
<span ref={editRef} {...buttonProps} className={readButton}>
{children}
</span>
) : (
<>
{children}
<IconButton
ref={editRef}
icon={<Icon name="edit" />}
aria-label="edit"
size="small"
onClick={handleEdit}
/>
</>
);

const renderInput = cloneElement(
input,
mergeProps(input.props, {
ref: inputRef,
defaultValue,
onKeyDown: handleKeyDown,
'aria-label': ariaLabel,
})
);

return isEditing ? (
<div className={cx(container, inline({ layout }))} data-test-id={testId}>
{renderInput}
<ButtonGroup spacing="compact">
<IconButton
kind="primary"
icon={<Icon name="check" />}
aria-label="save"
onClick={handleSave}
/>
<IconButton
kind="default"
icon={<Icon name="close" />}
aria-label="cancel"
className={cancelButton}
onClick={handleCancel}
/>
</ButtonGroup>
</div>
) : (
<div className={cx(!hideEdit && container)} data-test-id={testId}>
{renderReadContent}
</div>
);
};

export { InlineEdit };
export type { InlineEditProps };
2 changes: 2 additions & 0 deletions packages/inline-edit/src/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export type { InlineEditProps } from './InlineEdit';
export { InlineEdit } from './InlineEdit';
50 changes: 50 additions & 0 deletions packages/inline-edit/src/styles/InlineEdit.css.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import type { RecipeVariants } from '@vanilla-extract/recipes';

import { vars } from '@launchpad-ui/vars';
import { style } from '@vanilla-extract/css';
import { recipe } from '@vanilla-extract/recipes';

const container = style({
display: 'flex',
gap: vars.spacing[300],
alignItems: 'center',
});

const inline = recipe({
variants: {
layout: {
vertical: { flexDirection: 'column', alignItems: 'flex-start' },
horizontal: { flexDirection: 'row' },
},
},
});

const cancelButton = style({
selectors: {
'.Button--icon&': {
height: '3rem',
width: '3rem',
},
},
});

const readButton = style({
display: 'inline-block',
padding: `${vars.spacing[200]} ${vars.spacing[300]}`,
borderRadius: vars.border.radius.regular,
':hover': {
background: vars.color.bg.interactive.tertiary.hover,
cursor: 'pointer',
},
':focus-visible': {
borderRadius: vars.border.radius.medium,
boxShadow: `0 0 0 2px ${vars.color.bg.ui.primary}, 0 0 0 4px ${vars.color.shadow.interactive.focus}`,
outline: 0,
zIndex: 2,
},
});

type InlineVariants = RecipeVariants<typeof inline>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This Vanilla Extract utility returns type layout: "vertical" | "horizontal" for the prop which means we can have a single source of truth for props tied directly to component styles (variants).


export { container, cancelButton, inline, readButton };
export type { InlineVariants };
Loading
Loading