From 93f58fbe98bb177cc7a14b8990d38c6c1c6ed329 Mon Sep 17 00:00:00 2001 From: Ian Sanders Date: Mon, 15 Aug 2022 15:41:57 +0000 Subject: [PATCH 1/6] Limit the type of the `ref` forwarded to `MarkdownEditor` --- .../MarkdownEditor/MarkdownEditor.test.tsx | 22 ++++++++++++++++++- src/drafts/MarkdownEditor/MarkdownEditor.tsx | 20 +++++++++++++---- 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/src/drafts/MarkdownEditor/MarkdownEditor.test.tsx b/src/drafts/MarkdownEditor/MarkdownEditor.test.tsx index d52fa9ec66d..7c25dae3f1e 100644 --- a/src/drafts/MarkdownEditor/MarkdownEditor.test.tsx +++ b/src/drafts/MarkdownEditor/MarkdownEditor.test.tsx @@ -1,7 +1,7 @@ import {DiffAddedIcon} from '@primer/octicons-react' import {fireEvent, render as _render, waitFor, within} from '@testing-library/react' import userEvent from '@testing-library/user-event' -import React, {forwardRef, useLayoutEffect, useState} from 'react' +import React, {forwardRef, useLayoutEffect, useRef, useState} from 'react' import MarkdownEditor, {Emoji, MarkdownEditorHandle, MarkdownEditorProps, Mentionable, Reference, SavedReply} from '.' import ThemeProvider from '../../ThemeProvider' @@ -1104,4 +1104,24 @@ describe('MarkdownEditor', () => { expect(getInput()).toHaveFocus() }) }) + + it('uses types to prevent assigning HTMLTextAreaElement ref to MarkdownEditor', () => { + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const Element = () => { + const inputRef = useRef(null) + return ( + should not be assignable to Ref + ref={inputRef} + value="" + onChange={() => { + /*noop*/ + }} + onRenderPreview={async () => 'preview'} + > + Test + + ) + } + }) }) diff --git a/src/drafts/MarkdownEditor/MarkdownEditor.tsx b/src/drafts/MarkdownEditor/MarkdownEditor.tsx index ef8a7d7b32f..e85c8645cc5 100644 --- a/src/drafts/MarkdownEditor/MarkdownEditor.tsx +++ b/src/drafts/MarkdownEditor/MarkdownEditor.tsx @@ -97,11 +97,19 @@ export type MarkdownEditorProps = SxProp & { savedReplies?: SavedReply[] } +const handleBrand = Symbol() + export interface MarkdownEditorHandle { /** Focus on the markdown textarea (has no effect in preview mode). */ focus: (options?: FocusOptions) => void /** Scroll to the editor. */ scrollIntoView: (options?: ScrollIntoViewOptions) => void + /** + * This 'fake' member prevents other types from being assigned to this, thus + * disallowing broader ref types like `HTMLTextAreaElement`. + * @private + */ + [handleBrand]: undefined } const a11yOnlyStyle = {clipPath: 'Circle(0)', position: 'absolute'} as const @@ -171,10 +179,14 @@ const MarkdownEditor = forwardRef( }) const inputRef = useRef(null) - useImperativeHandle(ref, () => ({ - focus: opts => inputRef.current?.focus(opts), - scrollIntoView: opts => containerRef.current?.scrollIntoView(opts) - })) + useImperativeHandle( + ref, + () => + ({ + focus: opts => inputRef.current?.focus(opts), + scrollIntoView: opts => containerRef.current?.scrollIntoView(opts) + } as MarkdownEditorHandle) + ) const inputHeight = useRef(0) if (inputRef.current && inputRef.current.offsetHeight) inputHeight.current = inputRef.current.offsetHeight From 8721fd1aff049e1567f778f24c88ea8d9ca2fb20 Mon Sep 17 00:00:00 2001 From: Ian Sanders Date: Tue, 16 Aug 2022 20:50:50 +0000 Subject: [PATCH 2/6] Add cmd/ctrl+shift+P keyboard shortcut for toggling views --- .../MarkdownEditor/MarkdownEditor.test.tsx | 16 +++++++ src/drafts/MarkdownEditor/MarkdownEditor.tsx | 44 +++++++++++++++++-- src/drafts/MarkdownEditor/utils.ts | 5 +++ 3 files changed, 62 insertions(+), 3 deletions(-) diff --git a/src/drafts/MarkdownEditor/MarkdownEditor.test.tsx b/src/drafts/MarkdownEditor/MarkdownEditor.test.tsx index 7c25dae3f1e..a3f3d7ad16a 100644 --- a/src/drafts/MarkdownEditor/MarkdownEditor.test.tsx +++ b/src/drafts/MarkdownEditor/MarkdownEditor.test.tsx @@ -231,6 +231,22 @@ describe('MarkdownEditor', () => { expect(getInput()).toHaveAttribute('name', 'Name') }) + describe('toggles between view modes on ctrl/cmd+shift+P', () => { + const shortcut = '{Control>}{Shift>}{P}{/Control}{/Shift}' + + it('enters preview mode when editing', async () => { + const {getInput, user} = await render() + await user.type(getInput(), shortcut) + }) + + it('enters edit mode when previewing', async () => { + const {getInput, user, getViewSwitch} = await render() + await user.click(getViewSwitch()) + await user.keyboard(shortcut) + expect(getInput()).toHaveFocus() + }) + }) + describe('action buttons', () => { it('renders custom action buttons', async () => { const {getActionButton} = await render( diff --git a/src/drafts/MarkdownEditor/MarkdownEditor.tsx b/src/drafts/MarkdownEditor/MarkdownEditor.tsx index e85c8645cc5..91344050c44 100644 --- a/src/drafts/MarkdownEditor/MarkdownEditor.tsx +++ b/src/drafts/MarkdownEditor/MarkdownEditor.tsx @@ -1,4 +1,3 @@ -import {isMacOS} from '@primer/behaviors/utils' import {useSSRSafeId} from '@react-aria/ssr' import React, {forwardRef, useCallback, useEffect, useImperativeHandle, useMemo, useRef, useState} from 'react' import Box from '../../Box' @@ -24,6 +23,7 @@ import {SavedRepliesContext, SavedRepliesHandle, SavedReply} from './_SavedRepli import {Emoji} from './suggestions/_useEmojiSuggestions' import {Mentionable} from './suggestions/_useMentionSuggestions' import {Reference} from './suggestions/_useReferenceSuggestions' +import {isModifierKey} from './utils' export type MarkdownEditorProps = SxProp & { /** Current value of the editor as a multiline markdown string. */ @@ -119,6 +119,15 @@ const CONDENSED_WIDTH_THRESHOLD = 675 const {Slot, Slots} = createSlots(['Toolbar', 'Actions', 'Label']) export const MarkdownEditorSlot = Slot +/** + * We want to switch editors from preview mode on cmd/ctrl+shift+P. But in preview mode, + * there's no input to focus so we have to bind the event to the document. If there are + * multiple editors, we want the most recent one to switch to preview mode to be the one + * that we switch back to edit mode, so we maintain a LIFO stack of IDs of editors in + * preview mode. + */ +let editorsInPreviewMode: string[] = [] + /** * Markdown textarea with controls & keyboard shortcuts. */ @@ -245,7 +254,7 @@ const MarkdownEditor = forwardRef( savedRepliesRef.current?.openMenu() e.preventDefault() e.stopPropagation() - } else if (isMacOS() ? e.metaKey : e.ctrlKey) { + } else if (isModifierKey(e)) { if (e.key === 'Enter') onPrimaryAction?.() else if (e.key === 'b') format?.bold() else if (e.key === 'i') format?.italic() @@ -255,6 +264,7 @@ const MarkdownEditor = forwardRef( else if (e.key === '8') format?.unorderedList() else if (e.shiftKey && e.key === '7') format?.orderedList() else if (e.shiftKey && e.key === 'l') format?.taskList() + else if (e.shiftKey && e.key === 'p') setView?.('preview') else return e.preventDefault() @@ -266,6 +276,34 @@ const MarkdownEditor = forwardRef( } ) + useEffect(() => { + if (view === 'preview') { + editorsInPreviewMode.push(id) + + const handler = (e: KeyboardEvent) => { + if ( + !e.defaultPrevented && + editorsInPreviewMode.at(-1) === id && + isModifierKey(e) && + e.shiftKey && + e.key === 'p' + ) { + setView?.('edit') + requestAnimationFrame(() => inputRef.current?.focus()) + e.preventDefault() + } + } + document.addEventListener('keydown', handler) + + return () => { + document.removeEventListener('keydown', handler) + // Performing the filtering in the cleanup callback allows it to happen also when + // the user clicks the toggle button, not just on keyboard shortcut + editorsInPreviewMode = editorsInPreviewMode.filter(id_ => id_ !== id) + } + } + }, [view, setView, id]) + // If we don't memoize the context object, every child will rerender on every render even if memoized const context = useMemo( () => ({disabled, formattingToolsRef, condensed, required}), @@ -366,6 +404,7 @@ const MarkdownEditor = forwardRef( boxSizing: 'border-box' }} aria-live="polite" + tabIndex={-1} >

Rendered Markdown Preview

( ) } ) -MarkdownEditor.displayName = 'MarkdownEditor' export default MarkdownEditor diff --git a/src/drafts/MarkdownEditor/utils.ts b/src/drafts/MarkdownEditor/utils.ts index a78de05b5ab..71635c35d37 100644 --- a/src/drafts/MarkdownEditor/utils.ts +++ b/src/drafts/MarkdownEditor/utils.ts @@ -1,3 +1,5 @@ +import {isMacOS} from '@primer/behaviors/utils' + export const getSelectedLineRange = (textarea: HTMLTextAreaElement): [number, number] => { // Subtract one from the caret position so the newline found is not the one _at_ the caret position // then add one because we don't want to include the found newline. Also changes -1 (not found) result to 0 @@ -16,3 +18,6 @@ export const markdownLink = (text: string, url: string) => `[${text.replaceAll('[', '\\[').replaceAll(']', '\\]')}](${url.replaceAll('(', '\\(').replaceAll(')', '\\)')})` export const markdownImage = (altText: string, url: string) => `!${markdownLink(altText, url)}` + +export const isModifierKey = (event: KeyboardEvent | React.KeyboardEvent) => + isMacOS() ? event.metaKey : event.ctrlKey From 28e61fb431bbb2b673f08d49307a9c3e629bda1e Mon Sep 17 00:00:00 2001 From: Ian Sanders Date: Tue, 16 Aug 2022 20:51:41 +0000 Subject: [PATCH 3/6] Fix prop name typo in MarkdownViewer docs --- docs/content/drafts/MarkdownViewer.mdx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/content/drafts/MarkdownViewer.mdx b/docs/content/drafts/MarkdownViewer.mdx index 58d7e1d87a1..e5609ed156b 100644 --- a/docs/content/drafts/MarkdownViewer.mdx +++ b/docs/content/drafts/MarkdownViewer.mdx @@ -19,7 +19,7 @@ The `MarkdownViewer` displays rendered Markdown with appropriate styling and han const MarkdownViewerExample = () => { return ( // eslint-disable-next-line github/unescaped-html-literal - Lorem ipsum dolor sit amet.'}} /> + Lorem ipsum dolor sit amet.'}} /> ) } @@ -33,7 +33,7 @@ const MarkdownViewerExample = () => { return ( Example link"}} + dangerousRenderedHTML={{__html: "Example link"}} onLinkClick={ev => console.log(ev)} /> ) @@ -64,7 +64,7 @@ const renderedHtml = ` const MarkdownViewerExample = () => { return ( console.log(value) /* save the value to the server */} disabled={false} From 0f767c5a4a77d969f295f096810fd87958822276 Mon Sep 17 00:00:00 2001 From: Ian Sanders Date: Tue, 16 Aug 2022 17:03:55 -0400 Subject: [PATCH 4/6] Create improve-markdown-editor.md --- .changeset/improve-markdown-editor.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/improve-markdown-editor.md diff --git a/.changeset/improve-markdown-editor.md b/.changeset/improve-markdown-editor.md new file mode 100644 index 00000000000..d63ce4fa4c3 --- /dev/null +++ b/.changeset/improve-markdown-editor.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +Fix `MarkdownViewer` doc examples, add Cmd/Ctrl+Shift+P shortcut for toggling `MarkdownEditor` view mode, and strictly limit the type of the `ref` passed to `MarkdownEditor`. From 600dfff865b7348067a00141cdb51d46a6a88b55 Mon Sep 17 00:00:00 2001 From: Ian Sanders Date: Mon, 22 Aug 2022 09:19:11 -0400 Subject: [PATCH 5/6] Update src/drafts/MarkdownEditor/MarkdownEditor.tsx --- src/drafts/MarkdownEditor/MarkdownEditor.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/drafts/MarkdownEditor/MarkdownEditor.tsx b/src/drafts/MarkdownEditor/MarkdownEditor.tsx index 91344050c44..a304f94a2a0 100644 --- a/src/drafts/MarkdownEditor/MarkdownEditor.tsx +++ b/src/drafts/MarkdownEditor/MarkdownEditor.tsx @@ -289,7 +289,7 @@ const MarkdownEditor = forwardRef( e.key === 'p' ) { setView?.('edit') - requestAnimationFrame(() => inputRef.current?.focus()) + setTimeout => inputRef.current?.focus()) e.preventDefault() } } From bc5a62a32d44221b52657e85db0bc4de1b3d98a0 Mon Sep 17 00:00:00 2001 From: Ian Sanders Date: Tue, 23 Aug 2022 10:24:45 -0400 Subject: [PATCH 6/6] Fix syntax error --- src/drafts/MarkdownEditor/MarkdownEditor.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/drafts/MarkdownEditor/MarkdownEditor.tsx b/src/drafts/MarkdownEditor/MarkdownEditor.tsx index a304f94a2a0..061d083d231 100644 --- a/src/drafts/MarkdownEditor/MarkdownEditor.tsx +++ b/src/drafts/MarkdownEditor/MarkdownEditor.tsx @@ -289,7 +289,7 @@ const MarkdownEditor = forwardRef( e.key === 'p' ) { setView?.('edit') - setTimeout => inputRef.current?.focus()) + setTimeout(() => inputRef.current?.focus()) e.preventDefault() } }