-
Notifications
You must be signed in to change notification settings - Fork 6
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
Changes from 11 commits
2360d22
df0865e
97db03e
ec082e7
ccf3dc5
b709555
72b0a12
d8d3712
adbeb9b
c43735c
dac3053
e91ec6f
3fecbc0
e4de509
4262215
e502a8a
f9b9303
acdd668
f773bcc
33c40b3
a670f37
e56990a
53f36ba
721c835
5170e5f
9aaa2c5
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 |
---|---|---|
|
@@ -27,6 +27,7 @@ h1, | |
h2, | ||
h3 { | ||
color: var(--lp-color-text-ui-primary-base); | ||
margin: 0; | ||
} | ||
|
||
@font-face { | ||
|
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> | ||
); | ||
} |
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 | ||
``` |
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(); | ||
}); | ||
}); |
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(); | ||
}); | ||
}); |
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" | ||
} | ||
} |
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); | ||
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) => { | ||
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. Was exploring the new stories in this branch locally, noticed that pressing 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. 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. Yeah no strong opinion on blur implicitly cancelling. Can try that out and see how that feels. 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. 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.) 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. 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 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. 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.
Yeah. There are places where we use an inline edit to make a field opt-in, like flag keys upon creation.
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. 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.
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. 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. 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. 😅 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. Meeting with Antonio shortly and can go over this topic. 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. 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 }; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export type { InlineEditProps } from './InlineEdit'; | ||
export { InlineEdit } from './InlineEdit'; |
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>; | ||
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. This Vanilla Extract utility returns type |
||
|
||
export { container, cancelButton, inline, readButton }; | ||
export type { InlineVariants }; |
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.
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?