Query content model blocks. #2851
Conversation
…u/juliaroldi/images-word-tables
juliaroldi
requested review from
flyingbee2012,
BryanValverdeU,
JiuqingSong,
ianeli1 and
vinguyen12
| /** | ||
| * Options for queryContentModel | ||
| */ | ||
| export interface QueryContentModelOptions<T> { |
There was a problem hiding this comment.
add type constraint here for T.
| /** | ||
| * Optional selector to filter the blocks/segments | ||
| */ | ||
| selector?: (element: T) => boolean; |
There was a problem hiding this comment.
How about name it "filter"?
| elements.push(...(blockGroupsResults as T[])); | ||
| break; | ||
| case 'Table': | ||
| if (type == block.blockType && (!selector || selector(block as T))) { |
There was a problem hiding this comment.
what if T is a segment type, are we converting block to segment?
There was a problem hiding this comment.
Actually we should make sure only the correct type of object can trigger selector()
| const blocks: T[] = []; | ||
| for (const row of table.rows) { | ||
| for (const cell of row.cells) { | ||
| const items = queryContentModel<T>(cell, options); |
There was a problem hiding this comment.
Better create a internalQueryContentModel which does not need to check value of options then let both here and queryContentModel call it
packages/roosterjs-content-model-api/lib/modelApi/common/queryContentModel.ts
Outdated
Show resolved
Hide resolved
packages/roosterjs-content-model-api/lib/modelApi/common/queryContentModel.ts
Outdated
Show resolved
Hide resolved
juliaroldi
changed the title
| return elements; | ||
| } | ||
|
|
||
| function queryContentModelBlocksInternal<T extends ReadonlyContentModelBlock>( |
There was a problem hiding this comment.
since you don't need the parameter options now, you can merge these two functions together, so no need to duplicate the for loop for block group
| blockType?: ContentModelBlockType, | ||
| filter?: (element: T) => element is T, | ||
| findFirstOnly?: boolean | ||
| ): T[] { |
There was a problem hiding this comment.
Do you also want to return the path?
| block: ReadonlyContentModelBlock, | ||
| type: string | ||
| ): block is T { | ||
| return block.blockType == type; |
There was a problem hiding this comment.
No need to have this function. Just directly check blockType == type should work
| elements.push(block); | ||
| } | ||
|
|
||
| if (block.blockType == 'BlockGroup') { |
There was a problem hiding this comment.
Use switch...case instead of multiple if
| } | ||
|
|
||
| if (block.blockType == 'BlockGroup') { | ||
| for (const childBlock of block.blocks) { |
There was a problem hiding this comment.
What if you are querying block group? Then you should return directly here
| const table = block; | ||
| for (const row of table.rows) { | ||
| for (const cell of row.cells) { | ||
| for (const cellBlock of cell.blocks) { |
There was a problem hiding this comment.
cell is BlockGroup, so you can reuse the code for blockGroup here
| */ | ||
| export function queryContentModelBlocks<T extends ReadonlyContentModelBlock>( | ||
| group: ReadonlyContentModelBlockGroup, | ||
| blockType?: ContentModelBlockType, |
There was a problem hiding this comment.
This blockType needs to be coupled with T. Otherwise, you can pass two different types, for example:
queryContentModelBlock<ContentModelParagraph>(group, 'Table');In theory you should make this fail the build.
There was a problem hiding this comment.
You can declare like this.
export function queryContentModelBlocks<T extends ReadonlyContentModelBlock>(
group: ReadonlyContentModelBlockGroup,
blockType: T extends ReadonlyContentModelBlockBase<infer U> ? U : never,
...) {
...
}There was a problem hiding this comment.
Then blockType and T must match, otherwise build will fail.
This changed introduced a new API,
queryContentModel, to theroosterjs-content-model-api. Its role is to retrieve either blocks of a similar type, or a particular block specified by a selector. This enhancement builds upon thefindEditingImagefunction, enabling its logic to be applied in additional features like the OWA Accessibility Checker. Moreover, it addresses the issue of how images inserted into tables from Word Online are handled, asqueryContentModelalso checksformatContainerblocks during image searches.Test case: