Skip to content
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 Table first column #2652

Merged
merged 11 commits into from
May 23, 2024
13 changes: 0 additions & 13 deletions demo/scripts/controlsV2/demoButtons/setTableHeaderButton.ts

This file was deleted.

45 changes: 45 additions & 0 deletions demo/scripts/controlsV2/demoButtons/tableOptionsButton.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import { formatTable, getFormatState } from 'roosterjs-content-model-api';
import { TableMetadataFormat } from 'roosterjs-content-model-types';
import { TableOptionsMenuItemStringKey } from 'roosterjs-react';
import type { RibbonButton } from '../roosterjsReact/ribbon';

const TableEditOperationMap: Partial<Record<
TableOptionsMenuItemStringKey,
keyof TableMetadataFormat
>> = {
menuNameTableSetHeaderRow: 'hasHeaderRow',
menuNameTableSetFirstColumn: 'hasFirstColumn',
menuNameTableSetBandedColumns: 'hasBandedColumns',
menuNameTableSetBandedRows: 'hasBandedRows',
};

export const tableOptionsButton: RibbonButton<
'ribbonButtonTableOptions' | TableOptionsMenuItemStringKey
> = {
key: 'ribbonButtonTableOptions',
iconName: '',
unlocalizedText: 'Options',
isDisabled: formatState => !formatState.isInTable,
dropDownMenu: {
items: {
menuNameTableSetHeaderRow: 'Header Row',
menuNameTableSetFirstColumn: 'First Column',
menuNameTableSetBandedColumns: 'Banded Columns',
menuNameTableSetBandedRows: 'Banded Rows',
},
},
onClick: (editor, key) => {
if (key != 'ribbonButtonTableOptions') {
const format = getFormatState(editor);
const tableFormatProperty = TableEditOperationMap[key];
formatTable(
editor,
{ [tableFormatProperty]: !format.tableFormat[tableFormatProperty] },
true /*keepCellShade*/
);
}
},
commandBarProperties: {
iconOnly: false,
},
};
6 changes: 3 additions & 3 deletions demo/scripts/controlsV2/tabs/ribbonButtons.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ import { setBulletedListStyleButton } from '../demoButtons/setBulletedListStyleB
import { setHeadingLevelButton } from '../roosterjsReact/ribbon/buttons/setHeadingLevelButton';
import { setNumberedListStyleButton } from '../demoButtons/setNumberedListStyleButton';
import { setTableCellShadeButton } from '../demoButtons/setTableCellShadeButton';
import { setTableHeaderButton } from '../demoButtons/setTableHeaderButton';
import { spaceAfterButton, spaceBeforeButton } from '../demoButtons/spaceBeforeAfterButtons';
import { spacingButton } from '../demoButtons/spacingButton';
import { strikethroughButton } from '../roosterjsReact/ribbon/buttons/strikethroughButton';
Expand All @@ -47,6 +46,7 @@ import { tableBorderApplyButton } from '../demoButtons/tableBorderApplyButton';
import { tableBorderColorButton } from '../demoButtons/tableBorderColorButton';
import { tableBorderStyleButton } from '../demoButtons/tableBorderStyleButton';
import { tableBorderWidthButton } from '../demoButtons/tableBorderWidthButton';
import { tableOptionsButton } from '../demoButtons/tableOptionsButton';
import { tabNames } from './getTabs';
import { textColorButton } from '../roosterjsReact/ribbon/buttons/textColorButton';
import { underlineButton } from '../roosterjsReact/ribbon/buttons/underlineButton';
Expand Down Expand Up @@ -79,7 +79,7 @@ const tableButtons: RibbonButton<any>[] = [
insertTableButton,
formatTableButton,
setTableCellShadeButton,
setTableHeaderButton,
tableOptionsButton,
tableInsertButton,
tableDeleteButton,
tableBorderApplyButton,
Expand Down Expand Up @@ -169,7 +169,7 @@ const allButtons: RibbonButton<any>[] = [
listStartNumberButton,
formatTableButton,
setTableCellShadeButton,
setTableHeaderButton,
tableOptionsButton,
tableInsertButton,
tableDeleteButton,
tableMergeButton,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@ import {
extractBorderValues,
getFirstSelectedTable,
getSelectedCells,
hasMetadata,
parseValueWithUnit,
setFirstColumnFormatBorders,
updateTableCellMetadata,
updateTableMetadata,
} from 'roosterjs-content-model-dom';
import type {
IEditor,
Expand Down Expand Up @@ -366,6 +369,12 @@ export function applyTableBorderFormat(
modifyPerimeter(tableModel, sel, borderFormat, perimeter, isRtl);
}

const tableMeta = hasMetadata(tableModel) ? updateTableMetadata(tableModel) : {};
Andres-CT98 marked this conversation as resolved.
Show resolved Hide resolved
if (tableMeta) {
// Enforce first column format if necessary
setFirstColumnFormatBorders(tableModel.rows, tableMeta);
}

return true;
} else {
return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ describe('applyTableBorderFormat', () => {
format?: ContentModelTableCell['format']
) {
// Create a table with all cells selected except the first and last row and column
const table = createTable(rows);
const table: ContentModelTable = createTable(rows);
Andres-CT98 marked this conversation as resolved.
Show resolved Hide resolved
for (let i = 0; i < rows; i++) {
const row = table.rows[i];
for (let j = 0; j < columns; j++) {
Expand Down Expand Up @@ -55,6 +55,7 @@ describe('applyTableBorderFormat', () => {
const model = createContentModelDocument();
model.blocks.push(table);

console.log('FAILEDXXX', table.dataset);
Andres-CT98 marked this conversation as resolved.
Show resolved Hide resolved
let formatResult: boolean | undefined;

const formatContentModel = jasmine
Expand All @@ -77,6 +78,8 @@ describe('applyTableBorderFormat', () => {
blockGroupType: 'Document',
blocks: [expectedTable],
});

console.log('FAILED,', model, expectedTable);
Andres-CT98 marked this conversation as resolved.
Show resolved Hide resolved
}
it('All Borders', () => {
runTest(
Expand Down
2 changes: 1 addition & 1 deletion packages/roosterjs-content-model-dom/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ export { mergeModel } from './modelApi/editing/mergeModel';
export { deleteSelection } from './modelApi/editing/deleteSelection';
export { deleteSegment } from './modelApi/editing/deleteSegment';
export { deleteBlock } from './modelApi/editing/deleteBlock';
export { applyTableFormat } from './modelApi/editing/applyTableFormat';
export { applyTableFormat, setFirstColumnFormatBorders } from './modelApi/editing/applyTableFormat';
export { normalizeTable, MIN_ALLOWED_TABLE_CELL_WIDTH } from './modelApi/editing/normalizeTable';
export { setTableCellBackgroundColor } from './modelApi/editing/setTableCellBackgroundColor';
export { retrieveModelFormatState } from './modelApi/editing/retrieveModelFormatState';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export function applyTableFormat(

clearCache(rows);
formatCells(rows, effectiveMetadata, metaOverrides);
setFirstColumnFormat(rows, effectiveMetadata, metaOverrides);
setFirstColumnFormatBorders(rows, effectiveMetadata);
setHeaderRowFormat(rows, effectiveMetadata, metaOverrides);
return effectiveMetadata;
});
Expand Down Expand Up @@ -176,7 +176,7 @@ function formatCells(
format: TableMetadataFormat,
metaOverrides: MetaOverrides
) {
const { hasBandedRows, hasBandedColumns, bgColorOdd, bgColorEven } = format;
const { hasBandedRows, hasBandedColumns, bgColorOdd, bgColorEven, hasFirstColumn } = format;

rows.forEach((row, rowIndex) => {
row.cells.forEach((cell, colIndex) => {
Expand Down Expand Up @@ -212,14 +212,18 @@ function formatCells(

// Format Background Color
if (!metaOverrides.bgColorOverrides[rowIndex][colIndex]) {
const color =
hasBandedRows || hasBandedColumns
? (hasBandedColumns && colIndex % 2 != 0) ||
(hasBandedRows && rowIndex % 2 != 0)
? bgColorOdd
: bgColorEven
: bgColorEven; /* bgColorEven is the default color */

let color: string | null | undefined;
if (hasFirstColumn && colIndex == 0) {
color = null;
} else {
color =
hasBandedRows || hasBandedColumns
? (hasBandedColumns && colIndex % 2 != 0) ||
(hasBandedRows && rowIndex % 2 != 0)
? bgColorOdd
: bgColorEven
: bgColorEven; /* bgColorEven is the default color */
}
setTableCellBackgroundColor(
cell,
color,
Expand All @@ -236,28 +240,36 @@ function formatCells(
});
}

function setFirstColumnFormat(
/**
* Set the first column format borders for the table
* @param rows The rows of the table
* @param format The table metadata format
*/
export function setFirstColumnFormatBorders(
rows: ContentModelTableRow[],
format: Partial<TableMetadataFormat>,
metaOverrides: MetaOverrides
format: Partial<TableMetadataFormat>
) {
// Exit early hasFirstColumn is not set
if (!format.hasFirstColumn) {
return;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question, what if the table had first column before, then we want to use this function to remove the first column? If it just return here, will its border style remains no change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function is only intended to set the style, not remove it. The borders will go back to normal after any call to applyTableFormat. There is however an issue with isHeader, so that if 'First Column' is set, then unset, the cells will remain as headers. So I think change should be made in formatCells to make isHeader false for every cell, and then reapply it if hasFirstColumn or hasHeaderRow is true.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is my concern. I tried with your change, and after remove header or first column, the text remains in bold and in center. This does not happen before the change.

I don't know if that is an expected behavior, maybe confirm with Jorge.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is fixed on latest build.

}

rows.forEach((row, rowIndex) => {
row.cells.forEach((cell, cellIndex) => {
if (format.hasFirstColumn && cellIndex === 0) {
if (cellIndex == 0) {
cell.isHeader = true;

if (rowIndex !== 0 && !metaOverrides.bgColorOverrides[rowIndex][cellIndex]) {
setBorderColor(cell.format, 'borderTop');
setTableCellBackgroundColor(
cell,
null /*color*/,
false /*isColorOverride*/,
true /*applyToSegments*/
);
}

if (rowIndex !== rows.length - 1 && rowIndex !== 0) {
setBorderColor(cell.format, 'borderBottom');
switch (rowIndex) {
case 0:
setBorderColor(cell.format, 'borderBottom');
break;
case rows.length - 1:
setBorderColor(cell.format, 'borderTop');
break;
default:
setBorderColor(cell.format, 'borderTop');
setBorderColor(cell.format, 'borderBottom');
break;
}
} else {
cell.isHeader = false;
Expand All @@ -271,12 +283,17 @@ function setHeaderRowFormat(
format: TableMetadataFormat,
metaOverrides: MetaOverrides
) {
// Exit early if hasHeaderRow is not set
if (!format.hasHeaderRow) {
return;
Andres-CT98 marked this conversation as resolved.
Show resolved Hide resolved
}

const rowIndex = 0;

rows[rowIndex]?.cells.forEach((cell, cellIndex) => {
cell.isHeader = format.hasHeaderRow;
cell.isHeader = true;

if (format.hasHeaderRow && format.headerRowColor) {
if (format.headerRowColor) {
if (!metaOverrides.bgColorOverrides[rowIndex][cellIndex]) {
setTableCellBackgroundColor(
cell,
Expand All @@ -293,6 +310,11 @@ function setHeaderRowFormat(
});
}

/**
* @param format The cell format to set the border color
* @param key The border key to set the color
* @param value The color to set. If not given, it removes the color and sets the style to transparent
*/
function setBorderColor(format: BorderFormat, key: keyof BorderFormat, value?: string) {
const border = extractBorderValues(format[key]);
border.color = value || '';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export type TableMetadataFormat = {
* Table Borders Type. Use value of constant TableBorderFormat as value
*/
tableBorderFormat?: number;

/**
* Vertical alignment for each row
*/
Expand Down
1 change: 1 addition & 0 deletions packages/roosterjs-react/lib/contextMenu/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@ export {
TableEditShadeMenuItemStringKey,
TableEditMenuItemStringKey,
TableEditAlignTableMenuItemStringKey,
TableOptionsMenuItemStringKey,
} from './types/ContextMenuItemStringKeys';
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,15 @@ export type TableEditDeleteMenuItemStringKey =
| 'menuNameTableDeleteColumn'
| 'menuNameTableDeleteRow';

/**
* Key of localized strings of Table Options menu items
*/
export type TableOptionsMenuItemStringKey =
Andres-CT98 marked this conversation as resolved.
Show resolved Hide resolved
| 'menuNameTableSetHeaderRow'
| 'menuNameTableSetFirstColumn'
| 'menuNameTableSetBandedColumns'
| 'menuNameTableSetBandedRows';

/**
* Key of localized strings of Table Edit Merge menu item.
*/
Expand Down
Loading