Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Refactor table.sortableTable #10265

Merged
merged 1 commit into from
Aug 10, 2017
Merged

Refactor table.sortableTable #10265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions app/renderer/components/common/sortableTable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ const React = require('react')
const Immutable = require('immutable')
const tableSort = require('tablesort')

const {StyleSheet, css} = require('aphrodite/no-important')

// Utils
const cx = require('../../../../js/lib/classSet')
const eventUtil = require('../../../../js/lib/eventUtil')
Expand Down Expand Up @@ -418,16 +420,19 @@ class SortableTable extends React.Component {
return <tbody>{this.generateTableRows(this.props.rows)}</tbody>
}
}

render () {
if (!this.props.headings || !this.props.rows) {
return false
}

return <table
{...this.getTableAttributes()}
className={cx({
sort: !this.sortingDisabled,
sortableTable: !this.props.overrideDefaultStyle,
Copy link
Contributor Author

@luixxiul luixxiul Aug 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

overrideDefaultStyle is not used anywhere, so replaced it with true. To override the default style, instead you could use tableClassNames={css(styles.foo)}.

The class sortableTable is used heavily for automated tests so it is not removed yet.

[this.props.tableClassNames]: !!this.props.tableClassNames
sortableTable: true,
[this.props.tableClassNames]: !!this.props.tableClassNames,
[css(styles.table, this.props.fillAvailable && styles.table_fillAvailable)]: true
})}
ref={(node) => { this.table = node }}>
<thead>
Expand Down Expand Up @@ -469,4 +474,20 @@ class SortableTable extends React.Component {
}
}

const styles = StyleSheet.create({
// By default the width and margin are not specified.
// It can be specified by setting css to tableClassNames.
// See 'styles.devices__devicesList' on syncTab.js for example.
table: {
boxSizing: 'border-box',
cursor: 'default',
borderSpacing: 0
},

// Setting 'fillAvailable' maximizes the width of the table.
table_fillAvailable: {
width: '-webkit-fill-available'
}
})

module.exports = SortableTable
32 changes: 13 additions & 19 deletions app/renderer/components/preferences/extensionsTab.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class ExtensionsTab extends ImmutableComponent {

return [
{ // Icon
html: <img className={css(styles.table__row__column__icon)} src={this.getIcon(extension)} />
html: <img className={css(styles.tableRow__column__icon)} src={this.getIcon(extension)} />
},
{ // Name
html: <span data-extension-id={extension.get('id')}
Expand Down Expand Up @@ -91,12 +91,12 @@ class ExtensionsTab extends ImmutableComponent {

get columnClassNames () {
return [
css(styles.table__row__column, styles.table__row__column_center),
css(styles.table__row__column),
css(styles.table__row__column),
css(styles.table__row__column),
css(styles.table__row__column, styles.table__row__column_center)
// css(styles.table__row__column, styles.table__row__column_center)
css(styles.tableRow__column, styles.tableRow__column_center),
css(styles.tableRow__column),
css(styles.tableRow__column),
css(styles.tableRow__column),
css(styles.tableRow__column, styles.tableRow__column_center)
// css(styles.tableRow__column, styles.tableRow__column_center)
]
}

Expand All @@ -107,12 +107,12 @@ class ExtensionsTab extends ImmutableComponent {
return <section>
<DefaultSectionTitle data-l10n-id='extensions' />
<SortableTable
fillAvailable
sortingDisabled
tableClassNames={css(styles.table)}
headings={['icon', 'name', 'description', 'version', 'enabled'] /* 'exclude' */}
columnClassNames={this.columnClassNames}
rowClassNames={
this.props.extensions.map(entry => css(styles.table__row)).toJS()
this.props.extensions.map(entry => css(styles.tableRow)).toJS()
}
rows={this.props.extensions.map(entry => this.getRow(entry))} />
<footer className={css(styles.moreInfo)}>
Expand All @@ -129,13 +129,7 @@ class ExtensionsTab extends ImmutableComponent {
}

const styles = StyleSheet.create({
table: {
// TODO (Suguru): refactor sortableTable.js to remove !important
width: '100% !important',
marginBottom: '0 !important'
},
Copy link
Contributor Author

@luixxiul luixxiul Aug 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

table is no longer required thanks to fillAvailable. Also margin is no longer specified by default.

The table as such does not require margin in the same way as the buttons do not. If margin is required, it should be specified as an option; otherwise the margin should be added to the other element which exists around the table.


table__row: {
tableRow: {
fontSize: '15px',
background: '#fff',

Expand All @@ -151,15 +145,15 @@ const styles = StyleSheet.create({
}
},

table__row__column: {
tableRow__column: {
padding: '0 8px'
},

table__row__column_center: {
tableRow__column_center: {
textAlign: 'center'
},

table__row__column__icon: {
tableRow__column__icon: {
display: 'flex',
margin: 'auto',
width: '32px',
Expand Down
9 changes: 1 addition & 8 deletions app/renderer/components/preferences/payment/ledgerTable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ class LedgerTable extends ImmutableComponent {
</div>
</div>
<SortableTable
tableClassNames={css(styles.tableClass)}
fillAvailable
headings={['', 'publisher', 'include', 'views', 'timeSpent', 'percentage', 'actions']}
defaultHeading='percentage'
defaultHeadingSortOrder='desc'
Expand Down Expand Up @@ -339,13 +339,6 @@ const styles = StyleSheet.create({
}
},

tableClass: {
width: '100%',
borderCollapse: 'collapse',
border: 'none',
margin: '0 auto'
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See: https://github.com/brave/browser-laptop/pull/10265/files#diff-159ff36704e059402c3f17fb8a35a6fcR481


tableTh: {
color: paymentStylesVariables.tableHeader.fontColor,
fontSize: '14px',
Expand Down
8 changes: 3 additions & 5 deletions app/renderer/components/preferences/syncTab.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,8 @@ class SyncTab extends ImmutableComponent {
defaultHeading='syncDeviceLastActive'
defaultHeadingSortOrder='desc'
rows={this.devicesTableRows}
customCellClasses={css(styles.devices__devicesListCell)}
tableClassNames={css(styles.devices__devicesList)}
customCellClasses={css(styles.devices__devicesListCell)}
/>
</section>
}
Expand Down Expand Up @@ -590,10 +590,8 @@ const styles = StyleSheet.create({
},

devices__devicesList: {
// TODO: refactor sortableTable to remove !important
marginBottom: `${globalStyles.spacing.dialogInsideMargin} !important`,
boxSizing: 'border-box',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See https://github.com/brave/browser-laptop/pull/10265/files#diff-159ff36704e059402c3f17fb8a35a6fcR482

width: '600px !important'
marginBottom: globalStyles.spacing.dialogInsideMargin,
width: '600px'
},
devices__devicesListCell: {
padding: '4px 8px'
Expand Down
17 changes: 14 additions & 3 deletions js/about/preferences.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ const React = require('react')
const ImmutableComponent = require('../../app/renderer/components/immutableComponent')
const Immutable = require('immutable')
const UrlUtil = require('../lib/urlutil')
const {css} = require('aphrodite/no-important')
const {StyleSheet, css} = require('aphrodite/no-important')
const globalStyles = require('../../app/renderer/components/styles/global')
const commonStyles = require('../../app/renderer/components/styles/commonStyles')

// Components
Expand Down Expand Up @@ -323,8 +324,11 @@ class SearchTab extends ImmutableComponent {
<DefaultSectionTitle data-test-id='searchSettings' data-l10n-id='searchSettings' />
<SortableTable headings={['default', 'searchEngine', 'engineGoKey']} rows={this.searchProviders}
defaultHeading='searchEngine'
addHoverClass onClick={this.hoverCallback.bind(this)}
columnClassNames={['default', 'searchEngine', 'engineGoKey']} />
addHoverClass
onClick={this.hoverCallback.bind(this)}
tableClassNames={css(styles.sortableTable_searchTab)}
columnClassNames={['default', 'searchEngine', 'engineGoKey']}
/>
<DefaultSectionTitle data-l10n-id='locationBarSettings' />
<SettingsList>
<SettingCheckbox dataL10nId='showOpenedTabMatches' prefKey={settings.OPENED_TAB_SUGGESTIONS} settings={this.props.settings} onChangeSetting={this.props.onChangeSetting} />
Expand Down Expand Up @@ -941,6 +945,13 @@ class AboutPreferences extends React.Component {
}
}

const styles = StyleSheet.create({
sortableTable_searchTab: {
width: '704px',
marginBottom: globalStyles.spacing.settingsListContainerMargin // See syncTab.js for use cases
}
})

module.exports = {
AboutPreferences: <AboutPreferences />
}
2 changes: 0 additions & 2 deletions less/about/preferences.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,8 +247,6 @@ input[type="checkbox"][disabled] {
}

table.sortableTable {
width: 704px;
Copy link
Contributor Author

@luixxiul luixxiul Aug 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

width: 704px is used on about:preferences#search only so replaced it with sortableTable_searchTab. See above.


tbody td:first-of-type {
text-align: center;
color: @braveOrange;
Expand Down
5 changes: 0 additions & 5 deletions less/sortableTable.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,6 @@ table.sort {
}

table.sortableTable {
width: 100%;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is replaced with https://github.com/brave/browser-laptop/pull/10265/files#diff-159ff36704e059402c3f17fb8a35a6fcR489. The width is no longer specified by default (since it had to be overwritten with !important, which is redundant).

margin-bottom: 40px;
cursor: default;
border-spacing: 0;
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 3 properties above were replaced with https://github.com/brave/browser-laptop/pull/10265/files#diff-159ff36704e059402c3f17fb8a35a6fcR481.


tr {
height: 1.7em;
padding: 5px 20px;
Expand Down