Commit a4207c78 authored by manuk's avatar manuk Committed by Commit Bot

[chrome:omnibox] Refactor table cell rendering.

Previously, we used 6 satic methods to render each type of table cell:
3) renderBooleanProperty_
5) renderKeyValueTuples_
2) renderJsonProperty_
4) renderLinkProperty_
1) renderTextProperty_

This CL replaces those static methods with classes:
1) OutputBooleanProperty
2) OutputKeyValueTuplesProperty
3) OutputJsonProperty
4) OutputLinkProperty
5) OutputTextProperty

This refactor serves two purposes:

1) By having an object own the rendered HTML instead of directly injecting it into the DOM, we can later reference and manipulate these cells. E.g.:
  - When filtering, currently, we highlight entire rows if any of the cells contain a match. This change allows highlight the specific cells which matched.
  - Currently, when the user toggles `show details`, we must re-render the entire table, because we have no way of retrieving cells and toggling their visibility after we render them. This change allows toggling cell visibility after they've been added to the DOM, and, therefore, avoiding the re-rendering.

2) Supporting more intensive cells without relying on a network of static helper functions.
  - Currently, JSON content is stringified and displayed as text. This change allows more elegant, colored, tabular display of JSON property.
  - Currently, boolean cells have values true & false, making it hard to search for a specific column value. This would change allows filtering; e.g. 'is bookmarked'.
  - Currently, our cells contain mostly single text values. This change makes it easier to display multiple properties, images, and other formatted content in cells.

Bug: 891303
Change-Id: I89d75798943e135cda53a898a4b8c1f26dd869a1
Reviewed-on: https://chromium-review.googlesource.com/c/1354308
Commit-Queue: manuk hovanesian <manukh@chromium.org>
Reviewed-by: default avatarTommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612378}
parent 5ef4fe1c
...@@ -463,7 +463,7 @@ cr.define('omnibox_output', function() { ...@@ -463,7 +463,7 @@ cr.define('omnibox_output', function() {
// Reserve 1 more column if showing the additional properties column. // Reserve 1 more column if showing the additional properties column.
cell.colSpan = OutputMatch.displayedProperties(showDetails).length + cell.colSpan = OutputMatch.displayedProperties(showDetails).length +
showAdditionalPropertiesColumn; showAdditionalPropertiesColumn;
cell.textContent = this.matches[0].properties.providerName; cell.textContent = this.matches[0].properties.providerName.value;
row.appendChild(cell); row.appendChild(cell);
head.appendChild(row); head.appendChild(row);
return head; return head;
...@@ -481,8 +481,7 @@ cr.define('omnibox_output', function() { ...@@ -481,8 +481,7 @@ cr.define('omnibox_output', function() {
constructor(match) { constructor(match) {
/** @dict */ /** @dict */
this.properties = {}; this.properties = {};
/** @dict */ let unconsumedProperties = {};
this.additionalProperties = {};
Object.entries(match).forEach(propertyNameValueTuple => { Object.entries(match).forEach(propertyNameValueTuple => {
// TODO(manukh) replace with destructuring when the styleguide is // TODO(manukh) replace with destructuring when the styleguide is
// updated // updated
...@@ -491,13 +490,16 @@ cr.define('omnibox_output', function() { ...@@ -491,13 +490,16 @@ cr.define('omnibox_output', function() {
const propertyValue = propertyNameValueTuple[1]; const propertyValue = propertyNameValueTuple[1];
if (PROPERTY_OUTPUT_ORDER.some( if (PROPERTY_OUTPUT_ORDER.some(
displayProperty => property => property.propertyName === propertyName)) {
displayProperty.propertyName === propertyName)) { this.properties[propertyName] =
this.properties[propertyName] = propertyValue; OutputProperty.constructProperty(propertyName, propertyValue);
} else { } else {
this.additionalProperties[propertyName] = propertyValue; unconsumedProperties[propertyName] = propertyValue;
} }
}); });
/** @type {!OutputProperty} */
this.additionalProperties = OutputProperty.constructProperty(
'additionalProperties', unconsumedProperties);
/** @type {!Element} */ /** @type {!Element} */
this.associatedElement; this.associatedElement;
...@@ -511,139 +513,186 @@ cr.define('omnibox_output', function() { ...@@ -511,139 +513,186 @@ cr.define('omnibox_output', function() {
render(showDetails) { render(showDetails) {
const row = document.createElement('tr'); const row = document.createElement('tr');
OutputMatch.displayedProperties(showDetails) OutputMatch.displayedProperties(showDetails)
.map(property => { .map(property => this.properties[property.propertyName].render())
const value = this.properties[property.propertyName];
if (typeof value === 'boolean')
return OutputMatch.renderBooleanProperty_(value);
if (typeof value === 'object') {
// We check if the first element has key and value properties.
if (value && value[0] && value[0].key && value[0].value)
return OutputMatch.renderKeyValueTuples_(value);
else
return OutputMatch.renderJsonProperty_(value);
}
const LINK_REGEX = /^(http|https|ftp|chrome|file):\/\//;
if (LINK_REGEX.test(value))
return OutputMatch.renderLinkProperty_(value);
return OutputMatch.renderTextProperty_(value);
})
.forEach(cell => row.appendChild(cell)); .forEach(cell => row.appendChild(cell));
if (showDetails && this.hasAdditionalProperties) { if (showDetails && this.hasAdditionalProperties)
row.appendChild( row.appendChild(this.additionalProperties.render());
OutputMatch.renderJsonProperty_(this.additionalProperties));
}
this.associatedElement = row; this.associatedElement = row;
return this.associatedElement; return this.associatedElement;
} }
/** /**
* TODO(manukh) replace these static render_ functions with subclasses when * @param {string} name
* rendering becomes more substantial * @param {string=} url
* @private * @param {string=} tooltip
* @param {string} propertyValue
* @return {!Element} * @return {!Element}
*/ */
static renderTextProperty_(propertyValue) { static renderHeaderCell(name, url, tooltip) {
const cell = document.createElement('td'); const cell = document.createElement('th');
cell.textContent = propertyValue; if (url) {
const link = document.createElement('a');
link.textContent = name;
link.href = url;
cell.appendChild(link);
} else {
cell.textContent = name;
}
cell.className =
'column-' + name.replace(/[A-Z]/g, c => '-' + c.toLowerCase());
cell.title = tooltip || '';
return cell; return cell;
} }
/** /**
* @private * @return {!Array<!PresentationInfoRecord>} Array representing which
* @param {Object} propertyValue * columns need to be displayed.
*/
static displayedProperties(showDetails) {
return showDetails ?
PROPERTY_OUTPUT_ORDER :
PROPERTY_OUTPUT_ORDER.filter(property => property.displayAlways);
}
/**
* @return {boolean} Used to determine if the additional properties column
* needs to be displayed for this match.
*/
get hasAdditionalProperties() {
return Object.keys(this.additionalProperties).length > 0;
}
}
/** @abstract */
class OutputProperty {
/**
* @param {string} name
* @param {*} value
*/
constructor(name, value) {
/** @type {string} */
this.name = name;
/** @type {*} */
this.value = value;
}
/**
* @param {string} name
* @param {*} value
* @return {!OutputProperty}
*/
static constructProperty(name, value) {
if (typeof value === 'boolean')
return new OutputBooleanProperty(name, value);
if (typeof value === 'object')
// We check if the first element has key and value properties.
if (value && value[0] && value[0].key && value[0].value)
return new OutputKeyValueTuplesProperty(name, value);
else
return new OutputJsonProperty(name, value);
const LINK_REGEX = /^(http|https|ftp|chrome|file):\/\//;
if (LINK_REGEX.test(value))
return new OutputLinkProperty(name, value);
return new OutputTextProperty(name, value);
}
/**
* @abstract
* @return {!Element} * @return {!Element}
*/ */
static renderJsonProperty_(propertyValue) { render() {}
const cell = document.createElement('td');
const pre = document.createElement('pre'); /** @return {string} */
pre.textContent = JSON.stringify(propertyValue, null, 2); get text() {
cell.appendChild(pre); return this.value + '';
return cell;
} }
}
class OutputBooleanProperty extends OutputProperty {
/** /**
* @private * @override
* @param {boolean} propertyValue
* @return {!Element} * @return {!Element}
*/ */
static renderBooleanProperty_(propertyValue) { render() {
const cell = document.createElement('td'); const cell = document.createElement('td');
const icon = document.createElement('div'); const icon = document.createElement('div');
icon.className = propertyValue ? 'check-mark' : 'x-mark'; icon.className = this.value ? 'check-mark' : 'x-mark';
icon.textContent = propertyValue; icon.textContent = this.value;
cell.appendChild(icon); cell.appendChild(icon);
return cell; return cell;
} }
}
class OutputKeyValueTuplesProperty extends OutputProperty {
/** /**
* @private * @override
* @param {string} propertyValue
* @return {!Element} * @return {!Element}
*/ */
static renderLinkProperty_(propertyValue) { render() {
const cell = document.createElement('td'); const cell = document.createElement('td');
const link = document.createElement('a'); const pre = document.createElement('pre');
link.textContent = propertyValue; pre.textContent = this.text;
link.href = propertyValue; cell.appendChild(pre);
cell.appendChild(link);
return cell; return cell;
} }
/** /**
* @private * @override
* @param {Array<{key: string, value: string}>} propertyValue * @return {string}
* @return {Element}
*/ */
static renderKeyValueTuples_(propertyValue) { get text() {
return this.value.reduce(
(prev, {key, value}) => `${prev}${key}: ${value}\n`, '');
}
}
class OutputJsonProperty extends OutputProperty {
/**
* @override
* @return {!Element}
*/
render() {
const cell = document.createElement('td'); const cell = document.createElement('td');
const pre = document.createElement('pre'); const pre = document.createElement('pre');
const text = propertyValue.reduce( pre.textContent = this.text;
(prev, current) => `${prev}${current.key}: ${current.value}\n`, '');
pre.textContent = text;
cell.appendChild(pre); cell.appendChild(pre);
return cell; return cell;
} }
/** /**
* @param {string} name * @override
* @param {string=} url * @return {string}
* @param {string=} tooltip
* @return {!Element}
*/ */
static renderHeaderCell(name, url, tooltip) { get text() {
const cell = document.createElement('th'); return JSON.stringify(this.value, null, 2);
if (url) {
const link = document.createElement('a');
link.textContent = name;
link.href = url;
cell.appendChild(link);
} else {
cell.textContent = name;
}
cell.className =
'column-' + name.replace(/[A-Z]/g, c => '-' + c.toLowerCase());
cell.title = tooltip || '';
return cell;
} }
}
class OutputLinkProperty extends OutputProperty {
/** /**
* @return {!Array<!PresentationInfoRecord>} Array representing which columns * @override
* need to be displayed. * @return {!Element}
*/ */
static displayedProperties(showDetails) { render() {
return showDetails ? const cell = document.createElement('td');
PROPERTY_OUTPUT_ORDER : const link = document.createElement('a');
PROPERTY_OUTPUT_ORDER.filter(property => property.displayAlways); link.textContent = this.value;
link.href = this.value;
cell.appendChild(link);
return cell;
} }
}
class OutputTextProperty extends OutputProperty {
/** /**
* @return {boolean} Used to determine if the additional properties column * @override
* needs to be displayed for this match. * @return {!Element}
*/ */
get hasAdditionalProperties() { render() {
return Object.keys(this.additionalProperties).length > 0; const cell = document.createElement('td');
cell.textContent = this.value;
return cell;
} }
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment