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

fix(core/inputs): fix issues with calling onPathFocus for PT-input #5794

Merged
merged 3 commits into from
Feb 20, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {expect, test} from '@playwright/experimental-ct-react'
import {type Page} from '@playwright/test'
import {type Path, type SanityDocument} from '@sanity/types'

import {testHelpers} from '../../../utils/testHelpers'
import FocusTrackingStory from './FocusTrackingStory'

export type UpdateFn = () => {focusPath: Path; document: SanityDocument}
Expand Down Expand Up @@ -136,6 +137,24 @@ test.describe('Portable Text Input', () => {
await expect(blockObjectInput).not.toBeVisible()
})
})
test(`reports focus on spans with with .text prop, and everything else without`, async ({
mount,
page,
}) => {
const paths: Path[] = []
const pushPath = (path: Path) => paths.push(path)
await mount(<FocusTrackingStory document={document} onPathFocus={pushPath} />)
const {getFocusedPortableTextEditor} = testHelpers({page})
const $pte = await getFocusedPortableTextEditor('field-body')
Copy link
Member

Choose a reason for hiding this comment

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

Not a huge deal, but is there any particular meaning behind the $-prefix here? I guess it's because these refer to DOM elements, but my impression is that it's not a commonly used convention these days, since you can easily tell from the type anyway(?)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's to signal that it's a (synchronous) Locator instance.
The test suite started with this convention, and I think the Playwright documentation used to have this convention in their examples, but I can't find them anymore. I'm all in for not using them, but then we should follow the same convention everywhere within our test suite.

await expect($pte).toBeFocused()
expect(paths.slice(-1)[0]).toEqual(['body', {_key: 'a'}, 'children', {_key: 'b'}, 'text'])
const $inlineObject = page.getByTestId('inline-preview')
await $inlineObject.click()
expect(paths.slice(-1)[0]).toEqual(['body', {_key: 'g'}, 'children', {_key: 'i'}])
const $blockObject = page.getByTestId('pte-block-object')
await $blockObject.click()
expect(paths.slice(-1)[0]).toEqual(['body', {_key: 'k'}])
})
})

function waitForFocusedNodeText(page: Page, text: string) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,16 @@ const SCHEMA_TYPES = [

export function FocusTrackingStory({
focusPath,
onPathFocus,
document,
}: {
focusPath?: Path
onPathFocus?: (path: Path) => void
document?: SanityDocument
}) {
return (
<TestWrapper schemaTypes={SCHEMA_TYPES}>
<TestForm document={document} focusPath={focusPath} />
<TestForm document={document} focusPath={focusPath} onPathFocus={onPathFocus} />
</TestWrapper>
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,12 @@ declare global {

export function TestForm({
focusPath: focusPathFromProps,
onPathFocus: onPathFocusFromProps,
document: documentFromProps,
id: idFromProps = 'root',
}: {
focusPath?: Path
onPathFocus?: (path: Path) => void
document?: SanityDocument
id?: string
}) {
Expand Down Expand Up @@ -115,8 +117,9 @@ export function TestForm({
const handleFocus = useCallback(
(nextFocusPath: Path) => {
setFocusPath(nextFocusPath)
onPathFocusFromProps?.(nextFocusPath)
},
[setFocusPath],
[onPathFocusFromProps],
)

const handleBlur = useCallback(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,22 +146,27 @@ export function PortableTextInput(props: PortableTextInputProps) {
}
}, [hasFocusWithin])

// Report focus on spans with `.text` appended to the reported focusPath.
// This is done to support the Presentation tool which uses this kind of paths to refer to texts.
// The PT-input already supports these paths the other way around.
// It's a bit ugly right here, but it's a rather simple way to support the Presentation tool without
// having to change the PTE's internals.
const setFocusPathFromEditorSelection = useCallback(
(focusPath: Path) => {
// Report focus on spans with `.text` appended to the reported focusPath.
// This is done to support the Presentation tool which uses this kind of paths to refer to texts.
// The PT-input already supports these paths the other way around.
// It's a bit ugly right here, but it's a rather simple way to support the Presentation tool without
// having to change the PTE's internals.
if (
focusPath.length === 3 &&
focusPath[1] === 'children' &&
isKeySegment(focusPath[2]) &&
// Test if the focusPath is pointing directly to a span
const isSpanPath =
focusPath.length === 3 && // A span path is always 3 segments long
focusPath[1] === 'children' && // Is a child of a block
isKeySegment(focusPath[2]) && // Contains the key of the child
!portableTextMemberItems.some(
(item) => isKeySegment(focusPath[2]) && item.key === focusPath[2]._key,
) // Not an inline object
) {
(item) => isKeySegment(focusPath[2]) && item.member.key === focusPath[2]._key,
) // Not an inline object (it would be a member in this list, where spans are not). By doing this check we avoid depending on the value.
if (isSpanPath) {
// Append `.text` to the focusPath
onPathFocus(focusPath.concat('text'))
} else {
// Call normally
onPathFocus(focusPath)
}
},
[onPathFocus, portableTextMemberItems],
Expand Down
Loading