Refactor table.sortableTable #10265

luixxiul · 2017-08-03T13:12:48Z

Addresses #10263

Auditors: @cezaraugusto

Test Plan:

Open about:preferences#search
Make sure the custom width is applied to the table
Open about:preferences#sync
Make sure the custom width is applied to the table
Open about:preferences#extensions
Make sure '-webkit-fill-available' is applied to the table

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.

Test Plan:

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

Addresses #10263 Auditors: Test Plan: 1. Open about:preferences#search 2. Make sure the custom width is applied to the table 3. Open about:preferences#sync 4. Make sure the custom width is applied to the table 5. Open about:preferences#extensions 6. Make sure '-webkit-fill-available' is applied to the table

luixxiul · 2017-08-03T13:14:30Z

app/renderer/components/common/sortableTable.js

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


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.

luixxiul · 2017-08-03T13:15:32Z

app/renderer/components/preferences/extensionsTab.js

-    // TODO (Suguru): refactor sortableTable.js to remove !important
-    width: '100% !important',
-    marginBottom: '0 !important'
-  },


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.

luixxiul · 2017-08-03T13:16:11Z

app/renderer/components/preferences/payment/ledgerTable.js

-    borderCollapse: 'collapse',
-    border: 'none',
-    margin: '0 auto'
-  },


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

luixxiul · 2017-08-03T13:16:38Z

app/renderer/components/preferences/syncTab.js

@@ -590,10 +590,8 @@ const styles = StyleSheet.create({
  },

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


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

luixxiul · 2017-08-03T13:18:13Z

less/about/preferences.less

@@ -247,8 +247,6 @@ input[type="checkbox"][disabled] {
 }

 table.sortableTable {
-  width: 704px;


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

luixxiul · 2017-08-03T13:20:45Z

less/sortableTable.less

@@ -15,11 +15,6 @@ table.sort {
 }

 table.sortableTable {
-  width: 100%;


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).

luixxiul · 2017-08-03T13:21:30Z

less/sortableTable.less

-  width: 100%;
-  margin-bottom: 40px;
-  cursor: default;
-  border-spacing: 0;


the 3 properties above were replaced with https://github.com/brave/browser-laptop/pull/10265/files#diff-159ff36704e059402c3f17fb8a35a6fcR481.

luixxiul · 2017-08-03T13:22:41Z

I left some comments for code review :-)

codecov-io · 2017-08-03T13:22:55Z

Codecov Report

Merging #10265 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #10265      +/-   ##
==========================================
+ Coverage   52.95%   52.98%   +0.03%     
==========================================
  Files         228      228              
  Lines       20309    20308       -1     
  Branches     3254     3255       +1     
==========================================
+ Hits        10754    10760       +6     
+ Misses       9555     9548       -7

Flag	Coverage Δ
#unittest	`52.98% <100%> (+0.03%)`	⬆️

Impacted Files	Coverage Δ
app/renderer/components/preferences/syncTab.js	`17.01% <ø> (ø)`	⬆️
js/about/preferences.js	`43.56% <100%> (+0.77%)`	⬆️
...p/renderer/components/preferences/extensionsTab.js	`89.65% <100%> (ø)`	⬆️
...erer/components/preferences/payment/ledgerTable.js	`89.72% <100%> (ø)`	⬆️
app/renderer/components/common/sortableTable.js	`58.65% <100%> (+0.72%)`	⬆️

cezaraugusto

ledger table need some tweaks too. will that be addressed with #10238?

luixxiul · 2017-08-10T15:54:31Z

This time, unlike navigationBar, I am going to fix things gradually (as we have a plenty of time until 0.21 will be shipped).

With this PR I'm fixing <table>, not inside of it.

I will tweak stuff ledgerTable too, but only with other tables as well, to normalize the tables. Currently the styles of the table on about pages are inconsistent, and fixing it requires pre-works, one of which is this PR.

Also: #10238 does not polish the ledger table. Instead it fixes the layout of the element named walletBar, which no longer has <table> (instead it is styled with grid).

cezaraugusto

cool let's do it ++

luixxiul · 2017-08-10T16:13:00Z

thanks for review :-)

@cezaraugusto

Fixes brave#10434 Follow-up to brave#10265 Auditors: @cezaraugusto Test Plan: 1. Run `npm run add-simulated-synopsis-visits 100` 2. Open about:preferences#payments 3. Pin some sites 4. Make sure the orange border appears on the table

luixxiul added the refactoring/aphrodite label Aug 3, 2017

luixxiul added this to the 0.21.x (Nightly Channel) milestone Aug 3, 2017

luixxiul self-assigned this Aug 3, 2017

luixxiul requested a review from cezaraugusto August 3, 2017 13:12

luixxiul mentioned this pull request Aug 3, 2017

Refactor sortableTable with Aphrodite #10263

Closed

luixxiul commented Aug 3, 2017

View reviewed changes

luixxiul closed this Aug 3, 2017

luixxiul reopened this Aug 3, 2017

This was referenced Aug 3, 2017

Update extensionsTab to the modified BEM style #10208

Merged

Fix the wrong spacing around the per cent number on ledger panel #10356

Closed

cezaraugusto reviewed Aug 10, 2017

View reviewed changes

cezaraugusto approved these changes Aug 10, 2017

View reviewed changes

cezaraugusto merged commit 78a92da into brave:master Aug 10, 2017

This was referenced Aug 11, 2017

ledgerTable with pinned sites does not have the orange border #10434

Closed

Enable border on sortableTable by default #10435

Merged

tables on about:history and about:bookmarks does not have 100% width #10450

Closed

NejcZdovc mentioned this pull request Aug 14, 2017

Bookmark table is not full width #10474

Closed

bbondy removed this from the 0.21.x (Developer Channel) milestone Oct 25, 2017

bbondy added this to the 0.20.x (Beta Channel) milestone Oct 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor table.sortableTable #10265

Refactor table.sortableTable #10265

luixxiul commented Aug 3, 2017 •

edited

Loading

luixxiul Aug 3, 2017 •

edited

Loading

luixxiul Aug 3, 2017 •

edited

Loading

luixxiul Aug 3, 2017

luixxiul Aug 3, 2017

luixxiul Aug 3, 2017 •

edited

Loading

luixxiul Aug 3, 2017

luixxiul Aug 3, 2017

luixxiul commented Aug 3, 2017

codecov-io commented Aug 3, 2017

cezaraugusto left a comment •

edited

Loading

luixxiul commented Aug 10, 2017

cezaraugusto left a comment

luixxiul commented Aug 10, 2017

Refactor table.sortableTable #10265

Refactor table.sortableTable #10265

Conversation

luixxiul commented Aug 3, 2017 • edited Loading

luixxiul Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Aug 3, 2017

Choose a reason for hiding this comment

luixxiul Aug 3, 2017

Choose a reason for hiding this comment

luixxiul Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Aug 3, 2017

Choose a reason for hiding this comment

luixxiul Aug 3, 2017

Choose a reason for hiding this comment

luixxiul commented Aug 3, 2017

codecov-io commented Aug 3, 2017

Codecov Report

cezaraugusto left a comment • edited Loading

Choose a reason for hiding this comment

luixxiul commented Aug 10, 2017

cezaraugusto left a comment

Choose a reason for hiding this comment

luixxiul commented Aug 10, 2017

luixxiul commented Aug 3, 2017 •

edited

Loading

luixxiul Aug 3, 2017 •

edited

Loading

luixxiul Aug 3, 2017 •

edited

Loading

luixxiul Aug 3, 2017 •

edited

Loading

cezaraugusto left a comment •

edited

Loading