Skip to content

Commit

Permalink
Code cleanup: Remove TODO (microsoft#2384)
Browse files Browse the repository at this point in the history
  • Loading branch information
@JiuqingSong
JiuqingSong authored Feb 1, 2024
1 parent caee33f commit 17f4c87
Show file tree
Hide file tree
Showing 21 changed files with 192 additions and 77 deletions.
1 change: 0 additions & 1 deletion packages-content-model/roosterjs-content-model-api/lib/modelApi/block/setModelDirection.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ function internalSetDirection(
format.direction = direction;

// Adjust margin when change direction
// TODO: make margin and padding direction-aware, like what we did for textAlign. So no need to adjust them here
const marginLeft = format.marginLeft;
const paddingLeft = format.paddingLeft;

Expand Down
9 changes: 6 additions & 3 deletions ...ges-content-model/roosterjs-content-model-api/lib/modelApi/block/toggleModelBlockQuote.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ import type {
*/
export function toggleModelBlockQuote(
model: ContentModelDocument,
format: ContentModelFormatContainerFormat
formatLtr: ContentModelFormatContainerFormat,
formatRtl: ContentModelFormatContainerFormat
): boolean {
const paragraphOfQuote = getOperationalBlocks<
ContentModelFormatContainer | ContentModelListItem
Expand All @@ -30,12 +31,14 @@ export function toggleModelBlockQuote(
});
} else {
const step1Results: WrapBlockStep1Result<ContentModelFormatContainer>[] = [];
const creator = () => createFormatContainer('blockquote', format);
const creator = (isRtl: boolean) =>
createFormatContainer('blockquote', isRtl ? formatRtl : formatLtr);
const canMerge = (
isRtl: boolean,
target: ContentModelBlock,
current?: ContentModelFormatContainer
): target is ContentModelFormatContainer =>
canMergeQuote(target, current?.format || format);
canMergeQuote(target, current?.format || (isRtl ? formatRtl : formatLtr));

paragraphOfQuote.forEach(({ block, parent }) => {
if (isQuote(block)) {
Expand Down
4 changes: 0 additions & 4 deletions ...content-model/roosterjs-content-model-api/lib/modelApi/common/retrieveModelFormatState.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,6 @@ export function retrieveModelFormatState(
firstTableContext = tableContext;
}
}

// TODO: Support Code block in format state for Content Model
},
{
includeListFormatHolder: 'never',
Expand Down Expand Up @@ -161,8 +159,6 @@ function retrieveSegmentFormat(
mergeValue(result, 'backgroundColor', mergedFormat.backgroundColor, isFirst);
mergeValue(result, 'textColor', mergedFormat.textColor, isFirst);
mergeValue(result, 'fontWeight', mergedFormat.fontWeight, isFirst);

//TODO: handle block owning segments with different line-heights
mergeValue(result, 'lineHeight', mergedFormat.lineHeight, isFirst);
}

Expand Down
21 changes: 13 additions & 8 deletions packages-content-model/roosterjs-content-model-api/lib/modelApi/common/wrapBlock.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,19 @@ export function wrapBlockStep1<T extends ContentModelBlockGroup & ContentModelBl
step1Result: WrapBlockStep1Result<T>[],
parent: ContentModelBlockGroup | null,
blockToWrap: ContentModelBlock,
creator: () => T,
canMerge: (target: ContentModelBlock) => target is T
creator: (isRtl: boolean) => T,
canMerge: (isRtl: boolean, target: ContentModelBlock) => target is T
) {
const index = parent?.blocks.indexOf(blockToWrap) ?? -1;

if (parent && index >= 0) {
parent.blocks.splice(index, 1);

const prevBlock = parent.blocks[index - 1];
const wrapper = canMerge(prevBlock) ? prevBlock : createAndAdd(parent, index, creator);
const prevBlock: ContentModelBlock = parent.blocks[index - 1];
const isRtl = blockToWrap.format.direction == 'rtl';
const wrapper = canMerge(isRtl, prevBlock)
? prevBlock
: createAndAdd(parent, index, creator, isRtl);

setParagraphNotImplicit(blockToWrap);
addBlock(wrapper, blockToWrap);
Expand All @@ -40,13 +43,14 @@ export function wrapBlockStep1<T extends ContentModelBlockGroup & ContentModelBl
*/
export function wrapBlockStep2<T extends ContentModelBlockGroup & ContentModelBlock>(
step1Result: WrapBlockStep1Result<T>[],
canMerge: (target: ContentModelBlock, current: T) => target is T
canMerge: (isRtl: boolean, target: ContentModelBlock, current: T) => target is T
) {
step1Result.forEach(({ parent, wrapper }) => {
const index = parent.blocks.indexOf(wrapper);
const nextBlock = parent.blocks[index + 1];
const isRtl = wrapper.format.direction == 'rtl';

if (index >= 0 && canMerge(nextBlock, wrapper)) {
if (index >= 0 && canMerge(isRtl, nextBlock, wrapper)) {
wrapper.blocks.forEach(setParagraphNotImplicit);
wrapper.blocks.push(...nextBlock.blocks);
parent.blocks.splice(index + 1, 1);
Expand All @@ -57,9 +61,10 @@ export function wrapBlockStep2<T extends ContentModelBlockGroup & ContentModelBl
function createAndAdd<T extends ContentModelBlockGroup & ContentModelBlock>(
parent: ContentModelBlockGroup,
index: number,
creator: () => T
creator: (isRtl: boolean) => T,
isRtl: boolean
): T {
const block = creator();
const block = creator(isRtl);

parent.blocks.splice(index, 0, block);
return block;
Expand Down
2 changes: 0 additions & 2 deletions packages-content-model/roosterjs-content-model-api/lib/modelApi/list/setListType.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,6 @@ export function setListType(model: ContentModelDocument, listType: 'OL' | 'UL')
}
);

// Since there is only one paragraph under the list item, no need to keep its paragraph element (DIV).
// TODO: Do we need to keep the CSS styles applied to original DIV?
if (block.blockType == 'Paragraph') {
block.isImplicit = true;
}
Expand Down
25 changes: 18 additions & 7 deletions packages-content-model/roosterjs-content-model-api/lib/publicApi/block/toggleBlockQuote.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,19 @@ import type {
IStandaloneEditor,
} from 'roosterjs-content-model-types';

const DefaultQuoteFormat: ContentModelFormatContainerFormat = {
borderLeft: '3px solid rgb(200, 200, 200)', // TODO: Support RTL
const DefaultQuoteFormatLtr: ContentModelFormatContainerFormat = {
borderLeft: '3px solid rgb(200, 200, 200)',
textColor: 'rgb(102, 102, 102)',
};
const DefaultQuoteFormatRtl: ContentModelFormatContainerFormat = {
borderRight: '3px solid rgb(200, 200, 200)',
textColor: 'rgb(102, 102, 102)',
};
const BuildInQuoteFormat: ContentModelFormatContainerFormat = {
marginTop: '1em',
marginBottom: '1em',
marginLeft: '40px',
marginRight: '40px',
paddingLeft: '10px',
};

/**
Expand All @@ -25,11 +28,19 @@ const BuildInQuoteFormat: ContentModelFormatContainerFormat = {
*/
export default function toggleBlockQuote(
editor: IStandaloneEditor,
quoteFormat: ContentModelFormatContainerFormat = DefaultQuoteFormat
quoteFormat?: ContentModelFormatContainerFormat,
quoteFormatRtl?: ContentModelFormatContainerFormat
) {
const fullQuoteFormat = {
const fullQuoteFormatLtr: ContentModelFormatContainerFormat = {
...BuildInQuoteFormat,
paddingLeft: '10px',
...(quoteFormat ?? DefaultQuoteFormatLtr),
};
const fullQuoteFormatRtl: ContentModelFormatContainerFormat = {
...BuildInQuoteFormat,
...quoteFormat,
paddingRight: '10px',
direction: 'rtl',
...(quoteFormatRtl ?? quoteFormat ?? DefaultQuoteFormatRtl),
};

editor.focus();
Expand All @@ -38,7 +49,7 @@ export default function toggleBlockQuote(
(model, context) => {
context.newPendingFormat = 'preserve';

return toggleModelBlockQuote(model, fullQuoteFormat);
return toggleModelBlockQuote(model, fullQuoteFormatLtr, fullQuoteFormatRtl);
},
{
apiName: 'toggleBlockQuote',
Expand Down
1 change: 0 additions & 1 deletion ...s-content-model/roosterjs-content-model-api/lib/publicApi/segment/changeCapitalization.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ export default function changeCapitalization(
break;

case 'sentence':
// TODO: Add rules on punctuation for internationalization - TASK 104769
const punctuationMarks = '[\\.\\!\\?]';
// Find a match of a word character either:
// - At the beginning of a string with or without preceding whitespace, for
Expand Down
117 changes: 105 additions & 12 deletions ...ontent-model/roosterjs-content-model-api/test/modelApi/block/toggleModelBlockQuoteTest.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ describe('toggleModelBlockQuote', () => {
it('empty model', () => {
const doc = createContentModelDocument();

toggleModelBlockQuote(doc, {});
toggleModelBlockQuote(doc, {}, {});

expect(doc).toEqual({
blockGroupType: 'Document',
Expand All @@ -27,7 +27,7 @@ describe('toggleModelBlockQuote', () => {
para.segments.push(text);
doc.blocks.push(para);

toggleModelBlockQuote(doc, {});
toggleModelBlockQuote(doc, {}, {});

expect(doc).toEqual({
blockGroupType: 'Document',
Expand Down Expand Up @@ -56,7 +56,7 @@ describe('toggleModelBlockQuote', () => {
doc.blocks.push(para);
text.isSelected = true;

toggleModelBlockQuote(doc, {});
toggleModelBlockQuote(doc, {}, {});

expect(doc).toEqual({
blockGroupType: 'Document',
Expand Down Expand Up @@ -99,7 +99,7 @@ describe('toggleModelBlockQuote', () => {
text1.isSelected = true;
text2.isSelected = true;

toggleModelBlockQuote(doc, {});
toggleModelBlockQuote(doc, {}, {});

expect(doc).toEqual({
blockGroupType: 'Document',
Expand Down Expand Up @@ -140,6 +140,99 @@ describe('toggleModelBlockQuote', () => {
});
});

it('Has multiple selection, with RTL, do not merge', () => {
const doc = createContentModelDocument();
const para1 = createParagraph();
const text1 = createText('test1');
const para2 = createParagraph();
const text2 = createText('test2');
const para3 = createParagraph();
const text3 = createText('test3');

para1.segments.push(text1);
para2.segments.push(text2);
para3.segments.push(text3);

para2.format.direction = 'rtl';

doc.blocks.push(para1, para2, para3);
text1.isSelected = true;
text2.isSelected = true;
text3.isSelected = true;

toggleModelBlockQuote(doc, {}, { direction: 'rtl' });

expect(doc).toEqual({
blockGroupType: 'Document',
blocks: [
{
blockType: 'BlockGroup',
blockGroupType: 'FormatContainer',
tagName: 'blockquote',
format: {},
blocks: [
{
blockType: 'Paragraph',
format: {},
segments: [
{
segmentType: 'Text',
text: 'test1',
format: {},
isSelected: true,
},
],
},
],
},
{
blockType: 'BlockGroup',
blockGroupType: 'FormatContainer',
tagName: 'blockquote',
format: {
direction: 'rtl',
},
blocks: [
{
blockType: 'Paragraph',
format: {
direction: 'rtl',
},
segments: [
{
segmentType: 'Text',
text: 'test2',
format: {},
isSelected: true,
},
],
},
],
},
{
blockType: 'BlockGroup',
blockGroupType: 'FormatContainer',
tagName: 'blockquote',
format: {},
blocks: [
{
blockType: 'Paragraph',
format: {},
segments: [
{
segmentType: 'Text',
text: 'test3',
format: {},
isSelected: true,
},
],
},
],
},
],
});
});

it('Has single selection, merge before', () => {
const doc = createContentModelDocument();
const para1 = createParagraph();
Expand All @@ -155,7 +248,7 @@ describe('toggleModelBlockQuote', () => {
doc.blocks.push(para2);
text2.isSelected = true;

toggleModelBlockQuote(doc, {});
toggleModelBlockQuote(doc, {}, {});

expect(doc).toEqual({
blockGroupType: 'Document',
Expand Down Expand Up @@ -210,7 +303,7 @@ describe('toggleModelBlockQuote', () => {
doc.blocks.push(quote);
text1.isSelected = true;

toggleModelBlockQuote(doc, {});
toggleModelBlockQuote(doc, {}, {});

expect(doc).toEqual({
blockGroupType: 'Document',
Expand Down Expand Up @@ -271,7 +364,7 @@ describe('toggleModelBlockQuote', () => {
doc.blocks.push(quote3);
text2.isSelected = true;

toggleModelBlockQuote(doc, {});
toggleModelBlockQuote(doc, {}, {});

expect(doc).toEqual({
blockGroupType: 'Document',
Expand Down Expand Up @@ -343,7 +436,7 @@ describe('toggleModelBlockQuote', () => {
doc.blocks.push(quote3);
text2.isSelected = true;

toggleModelBlockQuote(doc, {});
toggleModelBlockQuote(doc, {}, {});

expect(doc).toEqual({
blockGroupType: 'Document',
Expand Down Expand Up @@ -436,7 +529,7 @@ describe('toggleModelBlockQuote', () => {
text2.isSelected = true;
text3.isSelected = true;

toggleModelBlockQuote(doc, {});
toggleModelBlockQuote(doc, {}, {});

expect(doc).toEqual({
blockGroupType: 'Document',
Expand Down Expand Up @@ -512,7 +605,7 @@ describe('toggleModelBlockQuote', () => {

text1.isSelected = true;

toggleModelBlockQuote(doc, {});
toggleModelBlockQuote(doc, {}, {});

expect(doc).toEqual({
blockGroupType: 'Document',
Expand Down Expand Up @@ -575,7 +668,7 @@ describe('toggleModelBlockQuote', () => {
text1.isSelected = true;
text2.isSelected = true;

toggleModelBlockQuote(doc, {});
toggleModelBlockQuote(doc, {}, {});

expect(doc).toEqual({
blockGroupType: 'Document',
Expand Down Expand Up @@ -637,7 +730,7 @@ describe('toggleModelBlockQuote', () => {

text2.isSelected = true;

toggleModelBlockQuote(doc, {});
toggleModelBlockQuote(doc, {}, {});

expect(doc).toEqual({
blockGroupType: 'Document',
Expand Down
Loading

0 comments on commit 17f4c87

Please sign in to comment.