From f00dd4ef605283e14312bebfbdafdd5c9d4376fe Mon Sep 17 00:00:00 2001
From: Daniel Richards <daniel.richards@automattic.com>
Date: Tue, 30 Jul 2019 18:03:08 +0800
Subject: [PATCH] Address review feedback

Update params of getCellAttribute function

Do nothing if insertRows is unable to determine the correct cellCount for the insertion
---
 packages/block-library/src/table/edit.js      |  2 +-
 packages/block-library/src/table/editor.scss  | 16 ++-------
 packages/block-library/src/table/state.js     | 33 ++++++++++---------
 .../block-library/src/table/test/state.js     | 26 +++++++++++++--
 4 files changed, 45 insertions(+), 32 deletions(-)

diff --git a/packages/block-library/src/table/edit.js b/packages/block-library/src/table/edit.js
index afe9d1e8b2c9d2..cb7324a3efe77e 100644
--- a/packages/block-library/src/table/edit.js
+++ b/packages/block-library/src/table/edit.js
@@ -229,7 +229,7 @@ export class TableEdit extends Component {
 
 		const { attributes } = this.props;
 
-		return getCellAttribute( attributes, { ...selectedCell, attributeName: 'align' } );
+		return getCellAttribute( attributes, selectedCell, 'align' );
 	}
 
 	/**
diff --git a/packages/block-library/src/table/editor.scss b/packages/block-library/src/table/editor.scss
index 5ffbc02b60ea49..73df244f815ff8 100644
--- a/packages/block-library/src/table/editor.scss
+++ b/packages/block-library/src/table/editor.scss
@@ -41,19 +41,9 @@
 
 	td.is-selected,
 	th.is-selected {
-		position: relative;
-
-		&::before {
-			content: "";
-			display: block;
-			position: absolute;
-			pointer-events: none;
-			top: -1px;
-			left: -1px;
-			right: -1px;
-			bottom: -1px;
-			border: 2px solid $blue-medium-500;
-		}
+		border-color: $blue-medium-500;
+		box-shadow: inset 0 0 0 1px $blue-medium-500;
+		border-style: double;
 	}
 
 	&__cell-content {
diff --git a/packages/block-library/src/table/state.js b/packages/block-library/src/table/state.js
index 7d7096b6f62925..89fd1c0227ef35 100644
--- a/packages/block-library/src/table/state.js
+++ b/packages/block-library/src/table/state.js
@@ -49,20 +49,18 @@ export function getFirstRow( state ) {
 /**
  * Gets an attribute for a cell.
  *
- * @param {Object} state 			   	 Current table state.
- * @param {string} options.sectionName	 Section of the cell to update.
- * @param {number} options.rowIndex    	 Row index of the cell to update.
- * @param {number} options.columnIndex 	 Column index of the cell to update.
- * @param {number} options.attributeName The name of the attribute to get the value of.
+ * @param {Object} state 		 Current table state.
+ * @param {Object} cellLocation  The location of the cell
+ * @param {string} attributeName The name of the attribute to get the value of.
  *
  * @return {*} The attribute value.
  */
-export function getCellAttribute( state, {
-	sectionName,
-	rowIndex,
-	columnIndex,
-	attributeName,
-} ) {
+export function getCellAttribute( state, cellLocation, attributeName ) {
+	const {
+		sectionName,
+		rowIndex,
+		columnIndex,
+	} = cellLocation;
 	return get( state, [ sectionName, rowIndex, 'cells', columnIndex, attributeName ] );
 }
 
@@ -142,9 +140,9 @@ export function isCellSelected( cellLocation, selection ) {
 /**
  * Inserts a row in the table state.
  *
- * @param {Object} state                Current table state.
- * @param {string} options.sectionName  Section in which to insert the row.
- * @param {number} options.rowIndex     Row index at which to insert the row.
+ * @param {Object} state               Current table state.
+ * @param {string} options.sectionName Section in which to insert the row.
+ * @param {number} options.rowIndex    Row index at which to insert the row.
  *
  * @return {Object} New table state.
  */
@@ -154,7 +152,12 @@ export function insertRow( state, {
 	columnCount,
 } ) {
 	const firstRow = getFirstRow( state );
-	const cellCount = columnCount || get( firstRow, [ 'cells', 'length' ], 2 );
+	const cellCount = columnCount === undefined ? get( firstRow, [ 'cells', 'length' ] ) : columnCount;
+
+	// Bail early if the function cannot determine how many cells to add.
+	if ( ! cellCount ) {
+		return state;
+	}
 
 	return {
 		[ sectionName ]: [
diff --git a/packages/block-library/src/table/test/state.js b/packages/block-library/src/table/test/state.js
index e1a72ccff50755..b946d45fcb50b7 100644
--- a/packages/block-library/src/table/test/state.js
+++ b/packages/block-library/src/table/test/state.js
@@ -163,12 +163,12 @@ describe( 'getFirstRow', () => {
 
 describe( 'getCellAttribute', () => {
 	it( 'should get the cell attribute', () => {
-		const state = getCellAttribute( tableWithAttribute, {
+		const cellLocation = {
 			sectionName: 'body',
 			rowIndex: 1,
 			columnIndex: 1,
-			attributeName: 'testAttr',
-		} );
+		};
+		const state = getCellAttribute( tableWithAttribute, cellLocation, 'testAttr' );
 
 		expect( state ).toBe( 'testVal' );
 	} );
@@ -369,6 +369,26 @@ describe( 'insertRow', () => {
 
 		expect( state ).toEqual( expected );
 	} );
+
+	it( 'should have no effect if `columnCount` is not provided and the table has no existing rows', () => {
+		const existingState = { body: {} };
+		const newState = insertRow( existingState, {
+			sectionName: 'body',
+			rowIndex: 0,
+		} );
+
+		expect( newState ).toBe( existingState );
+	} );
+
+	it( 'should have no effect if `columnCount` is `0`', () => {
+		const state = insertRow( tableWithHead, {
+			sectionName: 'head',
+			rowIndex: 1,
+			columnCount: 0,
+		} );
+
+		expect( state ).toBe( tableWithHead );
+	} );
 } );
 
 describe( 'insertColumn', () => {