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

- fix table border #102

Merged
merged 5 commits into from
Sep 19, 2018
Merged

- fix table border #102

merged 5 commits into from
Sep 19, 2018

Conversation

Marc-Andre-Rivet
Copy link
Contributor

  • apply table outer styling on the outer cells instead of the containing div -- there can be mismatches between the actual table size and it's container's size
  • update default styling for frozen rows/columns to provide a better default xp to the user

Marc-André Rivet added 2 commits September 19, 2018 10:18
- update frozen top/left table styling
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-102 September 19, 2018 14:21 Inactive
@@ -1,6 +1,6 @@
{
"name": "dash-table",
"version": "3.0.0rc19",
"version": "3.0.0rc20",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

bump version

@@ -42,7 +42,7 @@ class App extends Component {
}
],
table_style: [
{ selector: '.dash-spreadsheet.freeze-left', rule: 'width: 1000px' }
{ selector: '.dash-spreadsheet.freeze-left', rule: 'width: 1000px; max-width: 1000px;' }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

make sure the tests run against a same-styled table after the default styling update

.inset-shadow(var(--border), 1px, 0px, -1px, -1px);
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depending on whether rows and/or columns are frozen, sub-tables may need to provide extra borders on the outer left and top cells.. these mixins just make it easier to reuse below

}

&.freeze-top {
height: 500px;
height: fit-content;
max-height: 500px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default the table will now match its content size up to a given maximum size if frozen


&.focused + td {
.inset-shadow(var(--border), 0px, 0px, -1px, -1px);
}
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 selector was now useless -- it applied to the standard style as a special case!

{...props2}
n_fixed_columns={1}
n_fixed_rows={1}
/>));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two sets of identical tests for all permutations of frozen/not frozen columns and rows, with a table that fills its allocated space and one that does not

CHANGELOG.md Outdated
Table styling has been changed for frozen rows and columns. Default styling change from:

- frozen rows: { width: 500px } to { width: fit-content, max-width: 500px }
Copy link
Member

Choose a reason for hiding this comment

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

invert max-width / max-height

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-102 September 19, 2018 16:02 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-102 September 19, 2018 16:42 Inactive
@valentijnnieman
Copy link
Contributor

Looking at #101, if I run the demo with {'selector': '.dash-spreadsheet.freeze-left', 'rule': 'width: 100%;'} and only 3 columns, it looks like :
screen_scrolling
Is that desired behaviour or should it expand to show all 3 columns? I guess you have to specify max-width too, just wondering if that's intended behaviour.

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!

@Marc-Andre-Rivet
Copy link
Contributor Author

@valentijnnieman: Yes. This is desired behavior. The default style being aplied is { width: fit-content; max-width: 500px } -- we use fit-content to make sure the scrollbar will be located at the end of the table, not at the end of the parent element and we use a default max-width just to make it obvious that there is scrolling for most cases -- this forces the user to override these settings, but without proper styling it will look like fixed rows/columns / scrolling is broken and it seemed a ok compromise.

We don't try to fit the viewport to the content size (e.g. 3 columns vs number of pixels) -- as all columns can have some arbitrary size, whatever behavior would end up being weird.

@valentijnnieman
Copy link
Contributor

@Marc-Andre-Rivet Ok, great, thanks for explaining!

Copy link
Member

@cldougl cldougl left a comment

Choose a reason for hiding this comment

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

💃 after checking changelog rows-width/col-height flip

@Marc-Andre-Rivet Marc-Andre-Rivet merged commit bb5bb39 into master Sep 19, 2018
@Marc-Andre-Rivet Marc-Andre-Rivet deleted the 3.0-issue101-table-border branch September 27, 2018 15:14
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.

4 participants