Content Model: Set alignment API #1650

juliaroldi · 2023-03-17T15:10:27Z

Add setModelAlignment api that supports align lists and tables.

JiuqingSong · 2023-03-17T17:48:04Z

packages/roosterjs-content-model/test/domToModel/processors/listItemProcessorTest.ts

@@ -35,7 +35,7 @@ describe('listItemProcessor', () => {
        });
    });

-    it('LI with display:block', () => {
+    xit('LI with display:block', () => {


do we really need to skip this test?

JiuqingSong · 2023-03-17T17:48:44Z

packages/roosterjs-content-model/lib/publicTypes/format/ContentModelListItemFormat.ts

+/**
+ * The format object for a list item in Content Model
+ */
+export type ContentModelListItemFormat = DirectionFormat;


Also need to export this type in index.ts

JiuqingSong · 2023-03-17T17:51:18Z

packages/roosterjs-content-model/lib/publicTypes/format/ContentModelFormatMap.ts

@@ -51,7 +52,7 @@ export interface ContentModelFormatMap {
    /**
     * Format type for listItem
     */
-    listItem: ContentModelListItemLevelFormat;
+    listItem: ContentModelListItemFormat;


hmm, I think we need to add a new item here, but not change the existing one.

listItem handles item level format, which is also stored in ContentModelListItemLevelFormat. So the new format is different, need another handler set.

JiuqingSong · 2023-03-17T17:52:43Z

packages/roosterjs-content-model/lib/modelToDom/handlers/handleListItem.ts

        applyFormat(li, context.formatAppliers.segment, listItem.formatHolder.format, context);
-        applyFormat(li, context.formatAppliers.listItem, level, context);
+        applyFormat(li, context.formatAppliers.listLevel, level, context);


listLevel should be only applied to OL/UL, but not LI.

JiuqingSong · 2023-03-17T17:53:39Z

packages/roosterjs-content-model/lib/modelApi/creators/createListItem.ts

@@ -8,14 +8,19 @@ import { createSelectionMarker } from './createSelectionMarker';
 */
 export function createListItem(
    levels: ContentModelListItemLevelFormat[],
-    format?: ContentModelSegmentFormat
+    format?: ContentModelSegmentFormat,
+    li?: HTMLLIElement


Let's try avoid passing in DOM element into creator. You can pass in its style value directly.

Actually this should be done by format handler, but don't handle format directly in creator

JiuqingSong · 2023-03-17T17:55:06Z

packages/roosterjs-content-model/lib/formatHandlers/defaultFormatHandlers.ts

@@ -94,7 +94,7 @@ const defaultFormatKeysPerCategory: {
    [key in keyof ContentModelFormatMap]: (keyof FormatHandlerTypeMap)[];
 } = {
    block: blockFormatHandlers,
-    listItem: ['listItemThread', 'listItemMetadata'],
+    listItem: ['listItemThread', 'listItemMetadata', 'direction'],


same here, let's add a new item for direction

JiuqingSong · 2023-03-17T17:59:56Z

packages/roosterjs-content-model/lib/domToModel/processors/listItemProcessor.ts

@@ -23,12 +23,14 @@ export const listItemProcessor: ElementProcessor<HTMLLIElement> = (group, elemen
                    context
                );

-                const listItem = createListItem(listFormat.levels, context.segmentFormat);
+                const listItem = createListItem(listFormat.levels, context.segmentFormat, element);
+                parseFormat(element, context.formatParsers.listItem, listItem.format, context);


same here, use a different format handler set.

JiuqingSong · 2023-03-17T18:00:22Z

packages/roosterjs-content-model/lib/domToModel/processors/listItemProcessor.ts

                listFormat.listParent!.blocks.push(listItem);

                parseFormat(
                    element,
-                    context.formatParsers.listItem,
+                    context.formatParsers.listLevel,


don't change this. listItem and listLevel are doing different job.

JiuqingSong · 2023-03-17T18:02:15Z

packages/roosterjs-content-model/lib/modelApi/block/setModelAlignment.ts

+        } else if (block) {
+            const { format } = block;
+            format.textAlign = newAligment;


Actually this part is the same with line 63. We can merge them

JiuqingSong · 2023-03-17T18:04:49Z

packages/roosterjs-content-model/test/domToModel/processors/listItemProcessorTest.ts

@@ -54,7 +54,7 @@ describe('listItemProcessor', () => {
                    blockType: 'BlockGroup',
                    blockGroupType: 'ListItem',
                    blocks: [],
-                    levels: [{ listType: 'UL', displayForDummyItem: 'block' }],


This shows the code has regression. displayForDummyItem needs to be 'block' here

…u/juliaroldi/set-alignment-content-model

JiuqingSong · 2023-03-20T17:36:15Z

packages/roosterjs-content-model/lib/modelApi/creators/createListItem.ts

@@ -8,14 +8,19 @@ import { createSelectionMarker } from './createSelectionMarker';
 */
 export function createListItem(
    levels: ContentModelListItemLevelFormat[],
-    format?: ContentModelSegmentFormat
+    format?: ContentModelSegmentFormat,
+    textAlign?: string


As I commented in another place, this should be done by format handler, but not through a parameter of creator.

One of the reason is, here you allow to pass a string, then later you convert the type to "start|end|center". What if people pass in some other string? We can't detect it and there is no build time error. So that will most likely cause bug. To solve it, people need to convert their string to the type we need. But that is already done in format handler, so it is a dup.

JiuqingSong · 2023-03-20T17:38:51Z

packages/roosterjs-content-model/lib/domToModel/processors/listItemProcessor.ts

+                const listItem = createListItem(
+                    listFormat.levels,
+                    context.segmentFormat,
+                    element.style.alignSelf


same here, leave it to format handler.

JiuqingSong · 2023-03-20T21:24:29Z

packages/roosterjs-content-model/lib/formatHandlers/list/listItemElementFormatHandler.ts

+export const listItemElementFormatHandler: FormatHandler<ContentModelListItemFormat> = {
+    parse: (format, element) => {
+        const alignment = element.style.alignSelf;
+        if (alignment && ['end', 'start', 'center'].indexOf(alignment) > -1) {


I think all the logic here is already included in directionFormatHandler, right? Then we don't need this handler at all.

JiuqingSong · 2023-03-21T04:24:59Z

packages/roosterjs-content-model/lib/publicTypes/format/ContentModelFormatMap.ts

@@ -48,7 +49,7 @@ export interface ContentModelFormatMap {
     */
    tableCell: ContentModelTableCellFormat;

-    /**
+    /*


why change here?

It happened when I was solving conflicts. I'll fix.

juliaroldi added 2 commits March 17, 2023 12:06

squash

4de4959

conflicts

4ea04ec

juliaroldi marked this pull request as ready for review March 17, 2023 15:13

JiuqingSong requested changes Mar 17, 2023

View reviewed changes

juliaroldi marked this pull request as draft March 17, 2023 18:37

refactor

3d29eed

juliaroldi marked this pull request as ready for review March 17, 2023 21:56

Merge branch 'master' of https://github.com/microsoft/roosterjs into …

a6cc71c

…u/juliaroldi/set-alignment-content-model

juliaroldi force-pushed the u/juliaroldi/set-alignment-content-model branch from b9ce59f to a6cc71c Compare March 17, 2023 21:57

juliaroldi added 2 commits March 20, 2023 12:00

fix test

2669067

fix fireforx test

3200651

JiuqingSong reviewed Mar 20, 2023

View reviewed changes

remove handler

56bb98d

juliaroldi force-pushed the u/juliaroldi/set-alignment-content-model branch from ac523ef to 56bb98d Compare March 20, 2023 21:49

fix conflicts

d9cf826

JiuqingSong reviewed Mar 21, 2023

View reviewed changes

remove change

9e9c57a

JiuqingSong approved these changes Mar 21, 2023

View reviewed changes

juliaroldi merged commit 527cc0a into master Mar 21, 2023

juliaroldi deleted the u/juliaroldi/set-alignment-content-model branch March 21, 2023 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Content Model: Set alignment API #1650

Content Model: Set alignment API #1650

juliaroldi commented Mar 17, 2023 •

edited

Loading

JiuqingSong Mar 17, 2023

JiuqingSong Mar 17, 2023

JiuqingSong Mar 17, 2023

JiuqingSong Mar 17, 2023

JiuqingSong Mar 17, 2023

JiuqingSong Mar 17, 2023

JiuqingSong Mar 17, 2023

JiuqingSong Mar 17, 2023

JiuqingSong Mar 17, 2023

JiuqingSong Mar 17, 2023

JiuqingSong Mar 17, 2023

JiuqingSong Mar 20, 2023

JiuqingSong Mar 20, 2023

JiuqingSong Mar 20, 2023

JiuqingSong Mar 20, 2023

JiuqingSong Mar 21, 2023

juliaroldi Mar 21, 2023

Content Model: Set alignment API #1650

Content Model: Set alignment API #1650

Conversation

juliaroldi commented Mar 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliaroldi commented Mar 17, 2023 •

edited

Loading