Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Issue 680 - row and Bootstrap interactions#844

Merged
Marc-Andre-Rivet merged 58 commits intodevfrom
680-bootstrap-row
Nov 11, 2020
Merged

Issue 680 - row and Bootstrap interactions#844
Marc-Andre-Rivet merged 58 commits intodevfrom
680-bootstrap-row

Conversation

@Marc-Andre-Rivet
Copy link
Copy Markdown
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet commented Oct 28, 2020

Fixes #680
Fixes #796

  • adds the table- prefix to the row and row-{number} classes
  • unapplies Bootstrap styling for radio inputs
  • unapplies line-height changes
  • updates the visual tests to use bootstrap

Also adds visual tests to the tests/selenium test run and adds the new percy/dash-table-e2e snapshots bucket

@Marc-Andre-Rivet Marc-Andre-Rivet marked this pull request as ready for review October 29, 2020 14:25
key={`r${rowIndex}`}
ref={`r${rowIndex}`}
className={`row row-${rowIndex}`}
className={`table-row table-row-${rowIndex}`}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixing the class collision with Bootstrap

}

.row-1 {
.table-row-1 {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same class collision with Bootstrap

import { storiesOf } from '@storybook/react';
import random from 'core/math/random';
import DataTable from 'dash-table/dash/DataTable';
import './common';
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could technically add this to only one test and it would be applied to all but it seemed better to duplicate the import just so it's clear that this is "expected".

@@ -0,0 +1,2 @@
import 'bootstrap';
import 'bootstrap/dist/css/bootstrap.css'; No newline at end of file
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Import all the bootstrap things. This is applied to all tests (whether they all import this file or not). As currently set up, this means that we are no longer testing a "vanilla" version of the table in these tests and we would be required to duplicate this test run to do so (e.g. an additional ~1600 snapshots per build)

There are some differences between vanilla and the bootstrap styled table, as far as I can tell the main difference seems to be a slightly different font size. Nothing that strikes as compromising the vanilla test cases.

options=request.config.hook.pytest_setup_options(),
download_path=tmpdir.mkdir("download").strpath,
percy_assets_root=request.config.getoption("percy_assets"),
download_path=tmpdir.mkdir("dt-download").strpath,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Table tests sometimes use dash_thread_server and dash_duo -- dash_duo specifically also creates a temporary folder, both fixtures clash and the tests end up throwing an error because the directory already exists. Pytest uses a py27 compatible object that does not expose exists_ok and hence the error can't otherwise be prevented.

**ops
)

app.layout = html.Div([DataTable(**props)])
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Initially wanted this test to show a unmodified table + a another table in an iframe with bootstrap styles applied to offer some comparison point during review (e.g. only the bootstrap version changed vs. both) but the iframe content is never visible.

Copy link
Copy Markdown
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 LGTM!

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.

DataTable Cannot Show Full Table at Left and Right Edge [BUG] Left-most column clipped / overflow hidden

2 participants