-
Notifications
You must be signed in to change notification settings - Fork 431
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
Conversation
…spans We forgot to call this normally for anything else than spans. Also added some code comments.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
Component Testing Report Updated Feb 19, 2024 7:32 PM (UTC)
|
No changes to documentation |
Test that spans are reported with .text and everything else not.
675fd22
to
291d6bb
Compare
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.
LGTM (just a minor question/comment). Thanks for adding tests! ❤️
const pushPath = (path: Path) => paths.push(path) | ||
await mount(<FocusTrackingStory document={document} onPathFocus={pushPath} />) | ||
const {getFocusedPortableTextEditor} = testHelpers({page}) | ||
const $pte = await getFocusedPortableTextEditor('field-body') |
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.
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(?)
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.
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.
Description
I discovered some issues in #5786.
onPathFocus
on anything else than spans.I have added some Playwright-CT tests for this functionality now that test the values emitted by
onPathFocus
for focus on spans, inline blocks, and block objects.What to review
The bi-directionality and production of paths for various types of content should now be fully covered by the tests.
Testing
Notes for release
N/A - should use notes from #5786