Skip to content
This repository was archived by the owner on Jun 4, 2024. It is now read-only.

Table style #193

Merged
merged 10 commits into from
Nov 1, 2018
Merged

Table style #193

merged 10 commits into from
Nov 1, 2018

Conversation

Marc-Andre-Rivet
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet commented Nov 1, 2018

Fixes the pure styling issues from #150 (not the filter inputs)

  • style_table default nested props
  • default font-family
  • grow instead of fit content
  • some styling files reorg

- style_table default nested props
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-193 November 1, 2018 14:21 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-193 November 1, 2018 14:24 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-193 November 1, 2018 14:41 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-193 November 1, 2018 14:55 Inactive
const tableStyle = this.tableStyle(style_table);
const tableStyle = R.mergeAll(
this.tableStyle(DEFAULT_STYLE, style_table)
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Table style is now a combination of the prop and some default style

@@ -52,7 +52,7 @@ export const defaultProps = {
},
navigation: 'page',

content_style: 'fit',
content_style: 'grow',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Default is now 'grow' -- a lot of tests are updated below to conserve behavior

@Marc-Andre-Rivet Marc-Andre-Rivet changed the title [WIP] Styles Table style Nov 1, 2018
Marc-André Rivet added 2 commits November 1, 2018 11:18
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-193 November 1, 2018 15:19 Inactive
@@ -167,12 +169,11 @@
}

.dash-spreadsheet-container {
.reset-css();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consolidating component.less into Table.less

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-193 November 1, 2018 15:32 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-193 November 1, 2018 15:35 Inactive
Copy link
Contributor

@valentijnnieman valentijnnieman left a comment

Choose a reason for hiding this comment

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

Looks good to me! Shouldn't you have some tests that use the new default styles though, i.e. "grow", instead of updating all tests to use "fit" instead?

Copy link
Member

@chriddyp chriddyp left a comment

Choose a reason for hiding this comment

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

💃 looks good to me

@Marc-Andre-Rivet
Copy link
Contributor Author

Marc-Andre-Rivet commented Nov 1, 2018

@valentijnnieman Good remark. Actually I only updated the tests where the width was important (a few fixtures, all the Width.*.percy and some specific dropdown ones -- there's quite a few that are using the default behavior) -- that being said, I just realized that I changed the Width.percentages tests and those should not have changed. Will make 'grow' explicit on those so at least some tests are using the non-default, non-fit path. Those tests are also not leveraging the new style API fully.

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-193 November 1, 2018 16:04 Inactive
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants