feat(RichTextInput): create new component#6318
Conversation
🦋 Changeset detectedLatest commit: 63e7331 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6318 +/- ##
==========================================
+ Coverage 83.30% 92.44% +9.13%
==========================================
Files 44 540 +496
Lines 683 10234 +9551
Branches 195 3913 +3718
==========================================
+ Hits 569 9461 +8892
- Misses 104 706 +602
- Partials 10 67 +57
... and 489 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
d4b5583 to
8b976b0
Compare
| {...props} | ||
| error={getError( | ||
| { | ||
| label: label ?? ariaLabel ?? name, |
There was a problem hiding this comment.
| label: label ?? ariaLabel ?? name, | |
| label: errorLabel ?? label ?? ariaLabel ?? name, |
errorLabel is a new prop of BaseFieldProps that can be used to customize the error message (see #6241)
| field.onChange(value) | ||
| onChange?.(value as PathValue<TFieldValues, Path<TFieldValues>>) | ||
| }} | ||
| value={field.value ?? ''} |
| ) | ||
| }, | ||
| ], | ||
| title: 'Form/Components/Fields/RichTextEditorField', |
There was a problem hiding this comment.
| title: 'Form/Components/Fields/RichTextEditorField', | |
| title: 'Form/Components/Compositions/RichTextEditorField', |
|
|
||
| export default { | ||
| component: RichTextEditor, | ||
| title: 'UI/Data Entry/RichTextEditor', |
There was a problem hiding this comment.
| title: 'UI/Data Entry/RichTextEditor', | |
| title: 'Compositions/RichTextEditor', |
| style={{ | ||
| minHeight, | ||
| ...(maxHeight ? { maxHeight } : {}), | ||
| }} |
There was a problem hiding this comment.
why not add this in richTextEditorStyle.docRegion paired with assignInlineVars?
| sentiment?: 'danger' | 'success' | 'neutral' | ||
| }) => ( | ||
| <Row gap="1" templateColumns="minmax(0, 1fr) min-content"> | ||
| <div> |
There was a problem hiding this comment.
i don't understand the role of this div
| }) | ||
|
|
||
| export const docRegion = style({ | ||
| lineHeight: 1.5, |
There was a problem hiding this comment.
| lineHeight: 1.5, | |
| lineHeight: theme.typography.body.lineHeight, |
82f8b17 to
82b9a91
Compare
| ) | ||
| }, | ||
| ], | ||
| title: 'Compositions/RichTextEditorField', |
There was a problem hiding this comment.
| title: 'Compositions/RichTextEditorField', | |
| title: 'Form/Components/Compositions/RichTextEditorField', |
| const maxHeight = | ||
| typeof maxRows === 'number' | ||
| ? `calc(${lineHeightEm}em * ${maxRows} + 2 * ${padding})` | ||
| : undefined |
There was a problem hiding this comment.
| const maxHeight = | |
| typeof maxRows === 'number' | |
| ? `calc(${lineHeightEm}em * ${maxRows} + 2 * ${padding})` | |
| : undefined | |
| const maxHeight = | |
| typeof maxRows === 'number' | |
| ? `calc(${lineHeightEm}em * ${maxRows} + 2 * ${padding})` | |
| : 'none' |
Just a suggestion
| ...assignInlineVars({ | ||
| [docRegionMaxHeightVar]: maxHeight ?? 'none', | ||
| }), | ||
| minHeight, |
There was a problem hiding this comment.
minHeight should be an inlineVar too
| const isInOrderedList = isSelectionInNodeType(editorState, orderedList) | ||
|
|
||
| return ( | ||
| <Stack alignItems="center" direction="row" gap={1}> |
There was a problem hiding this comment.
idéalement il faudrait un role="toolbar" comme dans l'exemple ProseMirror. La navigation clavier est aussi différente (flèches pour naviguer entre les boutons, et quand on clique dessus ça remet le focus dans l'éditeur).
| disabled={disabled} | ||
| size="small" | ||
| variant={isMarkActive(editorState, strongMark) ? 'filled' : 'ghost'} | ||
| onMouseDown={event => { |
There was a problem hiding this comment.
- utiliser onClick pour supporter le clavier, et qui est déclenché quand on relâche le clic. C'est plus courant que de faire l'action au MouseDown, avant d'avoir relâché
- pourquoi event.preventDefault() ?
- il faut un label explicite aux boutons, là c'est le nom de l'icône ("BoldIcon"). Je vois qu'on utilise par endroits des fichiers de traduction en.ts, tu pourrais faire pareil en attendant qu'on trouve une meilleure solution
There was a problem hiding this comment.
- Utiliser onMouseDown permet runCommand avant le changement de focus
- L'event.preventDefault permet d'empecher le comportement par defaut d'un clic sur un bouton qui est de prendre le focus
- Je rajoute les labels
| return ( | ||
| <Stack gap="0.5"> | ||
| {label ? ( | ||
| <Label htmlFor={id ?? localId} required={required}> |
There was a problem hiding this comment.
2 issues :
- the localId is not used on the editor so the label points to nothing
- a
labelelement cannot be linked to a div. You can use an id on the label andaria-labelledbyon the editor instead
| }), | ||
| className, | ||
| )} | ||
| data-disabled={disabled ? 'true' : undefined} |
There was a problem hiding this comment.
why do you need this data-disabled attribute ?
There was a problem hiding this comment.
Effectivement il n'est pas utile
| disabled?: boolean | ||
| sentiment?: 'danger' | 'success' | 'neutral' | ||
| }) => ( | ||
| <Row gap="1" templateColumns="minmax(0, 1fr) min-content"> |
There was a problem hiding this comment.
the container of the Notice message should have:
- an id paired with an
aria-describedbyattribute on the text editor to link the message to the editor - a role="status" which implicitely add an aria-live="polite" so that screen readers read the status message when it's updated
| onBlur={onBlur} | ||
| onFocus={onFocus} |
There was a problem hiding this comment.
the blur and focus events don't bubble, did you check that you actually receive them here ? I don't see them in the proseMirror doc
| </Form>, | ||
| ) | ||
|
|
||
| await userEvent.click(screen.getByText('Submit')) |
There was a problem hiding this comment.
always prefer getByRole, idem in the other tests below
| await userEvent.click(screen.getByText('Submit')) | |
| await userEvent.click(screen.getByRole('button', { name: 'Submit' })) |
| await waitFor(() => { | ||
| expect(onSubmit).toHaveBeenCalledOnce() | ||
| expect(onSubmit.mock.calls[0]?.[0]?.test).toEqual( | ||
| expect.stringContaining('This is an example'), |
There was a problem hiding this comment.
can you check the whole string ?
|
|
||
| await waitFor(() => { | ||
| expect(onSubmit).toHaveBeenCalledOnce() | ||
| expect(onSubmit.mock.calls[0]?.[0]?.test).toEqual( |
| export const Required: StoryFn< | ||
| ComponentProps<typeof RichTextEditorField> | ||
| > = args => ( | ||
| <Stack gap={1}> |
There was a problem hiding this comment.
you could add this Stack and gap in the Template story, and bind the Template markup here like the playground so that they look the same
6e9a1dc to
63e7331
Compare

Summary
Type
Summarize concisely:
What is expected?
RichTextInput: create componentRichTextInputandRichTextInputFieldThe following changes were made:
(Describe what you did)
Relevant logs and/or screenshots