-
Notifications
You must be signed in to change notification settings - Fork 8
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
Extract text content from blocks #3057
Extract text content from blocks #3057
Conversation
51d5035
to
4723b9d
Compare
4723b9d
to
79ee9ff
Compare
|
…siteBlockSelectField and createCompositeBlockSwitchField
packages/admin/blocks-admin/src/blocks/helpers/createCompositeBlockSelectField.tsx
Show resolved
Hide resolved
return []; | ||
} | ||
|
||
return block?.extractTextContents?.(blockState.props) ?? []; |
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.
Shouldn't this extract the content for all child blocks?
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.
No, I don't think so because the non-active blocks are basically invisible and shouldn't be considered as the content (same as invisible OptionalBlock).
This could actually be a problem for the translation case. Then you want to include invisible content. Maybe we need a setting for that in extractTextContents.
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.
Yes, maybe we need to add a flag indicating if the extracted text is currently visible.
packages/admin/blocks-admin/src/blocks/factories/createColumnsBlock.tsx
Outdated
Show resolved
Hide resolved
return []; | ||
} | ||
|
||
return decoratedBlock.extractTextContents?.(state.block) ?? []; |
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.
The block's visibility isn't considered here... is this correct?
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.
True, added in e2146b8
-> maybe we need an option to include invisible content for translation
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.
We'll probably need an API for text visibility. But since this targets a feature branch, we can merge this PR and add it in a follow-up PR.
Follow-up Task: https://vivid-planet.atlassian.net/browse/COM-1620 |
Description
Add
extractTextContents
method to blocksextractTextContents
can be used to extract plain text from blocks. This functionality is particularly useful for operations such as search indexing or using the content for LLM-based tasks.The method is optional for now, but it is recommended to implement it for all blocks and documents. The default behavior is to return
In this PR, I implement
extractTextContents
for all factories and blocks in the library and checked all blocks in Demo. As you can see in Demo, I think it won't be necessary to override the default implementation for most cases.Acceptance criteria
Example data + result
Test content (blocks)
Result
A real use case is implemented in #2614
Further information