You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/09/21 19:37:44 UTC
[GitHub] [incubator-superset] kgabryje opened a new pull request #10982: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
kgabryje opened a new pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982
### SUMMARY
The goal is to replace all uses of `reactable-arc` library with DataTable from superset-ui (which uses `react-table` under the hood) and keep the UI/UX mostly unchanged. The reason for that is that `reactable-arc` has been unsupported for 2 years now, uses deprecated lifecycle methods and is incompatible with Typescript.
This PR replaces `reactable-arc` in `ChangeDatasourceModal` component.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kgabryje commented on pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
kgabryje commented on pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#issuecomment-702367813
@rusackas I moved the styles to a styled component and used variables from theme. Can you take a look?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kgabryje commented on pull request #10982: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
kgabryje commented on pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#issuecomment-696328506
@rusackas Can you take a look please?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kgabryje commented on pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
kgabryje commented on pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#issuecomment-697536884
@rusackas @ktmud Thank you for your suggestions. Before you suggested using `styled` and styles from superset-ui, I wrote some css of my own that removed the excessive margin and added permanent scrollbar. Do you think I should refactor or can we go with what we currently have?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#issuecomment-697128984
You can wrap the `DataTable` in a styled div like the `TableChart` does: https://github.com/apache-superset/superset-ui/blob/516b4d57b88bbadfbabebdb84b4862ce210bd350/plugins/plugin-chart-table/src/TableChart.tsx#L285
Here is the CSS: https://github.com/apache-superset/superset-ui/blob/516b4d57b88bbadfbabebdb84b4862ce210bd350/plugins/plugin-chart-table/src/Styles.tsx#L3
`DataTable` is designed to be CSS-less so it is library-agnostic.
Separate tables are needed to sticky headers because the scrollable container must contain only the table body (otherwise we could just use `position: sticky` on the `th` cells)---which was the original behavior of `jquery.DataTables` this component was created to replace..
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#discussion_r497978765
##########
File path: superset-frontend/stylesheets/superset.less
##########
@@ -600,3 +600,55 @@ input[type='radio']:checked:after {
hr {
border-top: 1px solid @gray-light;
}
+
+.modal-fullscreen {
+ & > .modal-lg {
+ height: calc(100% - 60px);
+ & > .modal-content {
+ height: 100%;
+ display: flex;
+ flex-direction: column;
+ & > .modal-body {
+ display: flex;
+ flex-direction: column;
+ flex: 1;
+ }
+ }
+ }
+}
+
+.flex-1 {
+ flex: 1;
+}
+
+.sticky-table {
+ & > div {
+ & > div:nth-child(2) {
+ & > div:first-child {
+ & > table {
+ margin-bottom: 0;
+ th {
+ border-bottom-width: 1px;
+ }
+ }
+ }
+ & > div:nth-child(2) {
+ &::-webkit-scrollbar {
+ -webkit-appearance: none;
+ &:vertical {
+ width: 7px;
+ }
+ &:horizontal {
+ height: 7px;
+ }
+ }
+
+ &::-webkit-scrollbar-thumb {
+ border-radius: 4px;
+ background-color: rgba(0, 0, 0, 0.5);
+ -webkit-box-shadow: 0 0 1px rgba(255, 255, 255, 0.5);
Review comment:
With Emotion, this could be:
```
-webkit-box-shadow: 0 0 1px ${({ theme }) => theme.colors.secondary.light5}80;
```
Which should look the same, but be more theming friendly _(helooooo dark mode!)_
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kgabryje closed pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
kgabryje closed pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-commenter edited a comment on pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#issuecomment-696608614
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=h1) Report
> Merging [#10982](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/448a41a4e7563cafadea1e03feb5980151e8b56d?el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `57.14%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10982 +/- ##
==========================================
- Coverage 65.77% 65.76% -0.02%
==========================================
Files 816 816
Lines 38395 38405 +10
Branches 3607 3610 +3
==========================================
+ Hits 25256 25257 +1
- Misses 13031 13040 +9
Partials 108 108
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `56.84% <16.66%> (-0.07%)` | :arrow_down: |
| #javascript | `61.72% <57.14%> (-0.01%)` | :arrow_down: |
| #python | `61.42% <ø> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...-frontend/src/datasource/ChangeDatasourceModal.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10982/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2RhdGFzb3VyY2UvQ2hhbmdlRGF0YXNvdXJjZU1vZGFsLnRzeA==) | `71.21% <57.14%> (-3.79%)` | :arrow_down: |
| [...et-frontend/src/dashboard/components/Dashboard.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10982/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0Rhc2hib2FyZC5qc3g=) | `84.70% <0.00%> (-4.71%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=footer). Last update [448a41a...0e9c075](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kgabryje commented on pull request #10982: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
kgabryje commented on pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#issuecomment-696536022
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#discussion_r497978870
##########
File path: superset-frontend/stylesheets/superset.less
##########
@@ -600,3 +600,55 @@ input[type='radio']:checked:after {
hr {
border-top: 1px solid @gray-light;
}
+
+.modal-fullscreen {
+ & > .modal-lg {
+ height: calc(100% - 60px);
+ & > .modal-content {
+ height: 100%;
+ display: flex;
+ flex-direction: column;
+ & > .modal-body {
+ display: flex;
+ flex-direction: column;
+ flex: 1;
+ }
+ }
+ }
+}
+
+.flex-1 {
+ flex: 1;
+}
+
+.sticky-table {
+ & > div {
+ & > div:nth-child(2) {
+ & > div:first-child {
+ & > table {
+ margin-bottom: 0;
+ th {
+ border-bottom-width: 1px;
+ }
+ }
+ }
+ & > div:nth-child(2) {
+ &::-webkit-scrollbar {
+ -webkit-appearance: none;
+ &:vertical {
+ width: 7px;
+ }
+ &:horizontal {
+ height: 7px;
+ }
+ }
+
+ &::-webkit-scrollbar-thumb {
+ border-radius: 4px;
+ background-color: rgba(0, 0, 0, 0.5);
Review comment:
```suggestion
background-color: ${({ theme }) => theme.colors.secondary.dark2}80;
```
Should be more themable this way
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-commenter edited a comment on pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#issuecomment-696608614
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=h1) Report
> Merging [#10982](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/448a41a4e7563cafadea1e03feb5980151e8b56d?el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `57.14%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10982 +/- ##
==========================================
- Coverage 65.77% 65.76% -0.02%
==========================================
Files 816 816
Lines 38395 38405 +10
Branches 3607 3610 +3
==========================================
+ Hits 25256 25257 +1
- Misses 13031 13040 +9
Partials 108 108
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `56.84% <16.66%> (-0.07%)` | :arrow_down: |
| #javascript | `61.72% <57.14%> (-0.01%)` | :arrow_down: |
| #python | `61.42% <ø> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...-frontend/src/datasource/ChangeDatasourceModal.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10982/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2RhdGFzb3VyY2UvQ2hhbmdlRGF0YXNvdXJjZU1vZGFsLnRzeA==) | `71.21% <57.14%> (-3.79%)` | :arrow_down: |
| [...et-frontend/src/dashboard/components/Dashboard.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10982/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0Rhc2hib2FyZC5qc3g=) | `84.70% <0.00%> (-4.71%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=footer). Last update [448a41a...0e9c075](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kgabryje closed pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
kgabryje closed pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on pull request #10982: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
rusackas commented on pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#issuecomment-696410508
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#discussion_r497979734
##########
File path: superset-frontend/stylesheets/superset.less
##########
@@ -600,3 +600,55 @@ input[type='radio']:checked:after {
hr {
border-top: 1px solid @gray-light;
}
+
+.modal-fullscreen {
+ & > .modal-lg {
+ height: calc(100% - 60px);
+ & > .modal-content {
+ height: 100%;
+ display: flex;
+ flex-direction: column;
+ & > .modal-body {
+ display: flex;
+ flex-direction: column;
+ flex: 1;
+ }
+ }
+ }
+}
+
+.flex-1 {
+ flex: 1;
+}
+
+.sticky-table {
+ & > div {
+ & > div:nth-child(2) {
+ & > div:first-child {
+ & > table {
+ margin-bottom: 0;
+ th {
+ border-bottom-width: 1px;
+ }
+ }
+ }
+ & > div:nth-child(2) {
+ &::-webkit-scrollbar {
+ -webkit-appearance: none;
+ &:vertical {
+ width: 7px;
Review comment:
Not sure if this 7px measurement (and the one below it) need to be exactly 7px, but if it's just _about that big_ and 8px works, then if/win this is moved to Emotion I would do `width: ${({ theme }) => theme.gridUnit * 2}px` to make it themable
```
width: 7px;
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on a change in pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#discussion_r508233307
##########
File path: superset-frontend/src/datasource/ChangeDatasourceModal.tsx
##########
@@ -17,10 +17,9 @@
* under the License.
*/
import React, { FunctionComponent, useState, useRef } from 'react';
-// @ts-ignore
-import { Table } from 'reactable-arc';
+import DataTable from '@superset-ui/plugin-chart-table/lib/DataTable';
Review comment:
Yes, there was a plan to move it out. I never followed up with it because of priorities. Will try to get it done by the end of this year!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#discussion_r496281453
##########
File path: superset-frontend/stylesheets/superset.less
##########
@@ -600,3 +600,55 @@ input[type='radio']:checked:after {
hr {
border-top: 1px solid @gray-light;
}
+
+.modal-fullscreen {
+ & > .modal-lg {
+ height: calc(100% - 60px);
+ & > .modal-content {
+ height: 100%;
+ display: flex;
+ flex-direction: column;
+ & > .modal-body {
+ display: flex;
+ flex-direction: column;
+ flex: 1;
+ }
+ }
+ }
+}
+
+.flex-1 {
Review comment:
Is there a reason not to have this just be part of the .sticky-table class?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-commenter commented on pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#issuecomment-696608614
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=h1) Report
> Merging [#10982](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/448a41a4e7563cafadea1e03feb5980151e8b56d?el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `57.14%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10982 +/- ##
==========================================
- Coverage 65.77% 65.76% -0.02%
==========================================
Files 816 816
Lines 38395 38405 +10
Branches 3607 3610 +3
==========================================
+ Hits 25256 25257 +1
- Misses 13031 13040 +9
Partials 108 108
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `56.84% <16.66%> (-0.07%)` | :arrow_down: |
| #javascript | `61.72% <57.14%> (-0.01%)` | :arrow_down: |
| #python | `61.42% <ø> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...-frontend/src/datasource/ChangeDatasourceModal.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10982/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2RhdGFzb3VyY2UvQ2hhbmdlRGF0YXNvdXJjZU1vZGFsLnRzeA==) | `71.21% <57.14%> (-3.79%)` | :arrow_down: |
| [...et-frontend/src/dashboard/components/Dashboard.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10982/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0Rhc2hib2FyZC5qc3g=) | `84.70% <0.00%> (-4.71%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=footer). Last update [448a41a...0e9c075](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#discussion_r499191026
##########
File path: superset-frontend/src/datasource/ChangeDatasourceModal.tsx
##########
@@ -17,10 +17,9 @@
* under the License.
*/
import React, { FunctionComponent, useState, useRef } from 'react';
-// @ts-ignore
-import { Table } from 'reactable-arc';
+import DataTable from '@superset-ui/plugin-chart-table/lib/DataTable';
Review comment:
@kgabryje @ktmud Just a note that this is the second time this table has been imported/used. Anyone think it seems a little weird? I'm all for reusing code! Maybe it should be moved out of the table plugin and into a @superset-ui/core export, if this is going to happen more frequently. ¯\\\_(ツ)_/¯
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kgabryje commented on pull request #10982: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
kgabryje commented on pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#issuecomment-696536022
> Is it possible to get rid of the blank row and extra horizontal gray line at the top of the list?
>
> 
That is a strange behaviour of DataTable with `sticky` param - for some reason it wraps `<thead>` and `<tbody>` in separate `<table>` tags, and table has margin bottom set. That happens in `StickyWrap` component in `superset-ui/plugins/plugin-chart-table/src/DataTable/hooks/useSticky.tsx` file. I'll try to figure out how to prevent it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on pull request #10982: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
rusackas commented on pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#issuecomment-696411531
This cropped text is a little worrisome, too... when it's visible, it indicates that there's something to scroll (that's a good thing) but looks a little weird. I _assume_ that depending on the size of your window, you may not see that partial line, and therefore might have no idea that you should scroll.
So, could we maybe...
1) Make the modal fit the whole content area?
2) Show a scrollbar if it _can't_ fit?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on pull request #10982: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
rusackas commented on pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#issuecomment-696410508
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kgabryje edited a comment on pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
kgabryje edited a comment on pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#issuecomment-696590695
@rusackas I did some css tweaks. They're not pretty, but necessary to override some DataTable styles
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
rusackas commented on pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#issuecomment-700326371
> @rusackas @ktmud Thank you for your suggestions. Before you suggested using `styled` and styles from superset-ui, I wrote some css of my own that removed the excessive margin and added permanent scrollbar. Do you think I should refactor or can we go with what we currently have?
I would prefer the refactor if it's possible, for a few reasons:
1) We're trying to make the LESS codebase smaller, rather than add anything more to it (we'd like to one day remove these global file styles, if it becomes possible).
2) Add the styles within the components that they concern, so that future people can refactor/add/remove styles more easily (i.e. within the components, thus the shift to Emotion)
3) You could use Theme variables... in the CSS on this PR, for example, you could use the 4px `gridUnit`(or multiples of it) used in an increasing number of places in superset. The idea here is that no fonts/sizes/colors should be hard-coded in an effort to eventually make Superset more themeable.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on pull request #10982: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
rusackas commented on pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#issuecomment-696410508
Is it possible to get rid of the blank row and extra horizontal gray line at the top of the list?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#discussion_r497978870
##########
File path: superset-frontend/stylesheets/superset.less
##########
@@ -600,3 +600,55 @@ input[type='radio']:checked:after {
hr {
border-top: 1px solid @gray-light;
}
+
+.modal-fullscreen {
+ & > .modal-lg {
+ height: calc(100% - 60px);
+ & > .modal-content {
+ height: 100%;
+ display: flex;
+ flex-direction: column;
+ & > .modal-body {
+ display: flex;
+ flex-direction: column;
+ flex: 1;
+ }
+ }
+ }
+}
+
+.flex-1 {
+ flex: 1;
+}
+
+.sticky-table {
+ & > div {
+ & > div:nth-child(2) {
+ & > div:first-child {
+ & > table {
+ margin-bottom: 0;
+ th {
+ border-bottom-width: 1px;
+ }
+ }
+ }
+ & > div:nth-child(2) {
+ &::-webkit-scrollbar {
+ -webkit-appearance: none;
+ &:vertical {
+ width: 7px;
+ }
+ &:horizontal {
+ height: 7px;
+ }
+ }
+
+ &::-webkit-scrollbar-thumb {
+ border-radius: 4px;
+ background-color: rgba(0, 0, 0, 0.5);
Review comment:
with Emotion, this could be:
```
background-color: ${({ theme }) => theme.colors.secondary.dark2}80;
```
Which would be more themable
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#discussion_r497979150
##########
File path: superset-frontend/stylesheets/superset.less
##########
@@ -600,3 +600,55 @@ input[type='radio']:checked:after {
hr {
border-top: 1px solid @gray-light;
}
+
+.modal-fullscreen {
+ & > .modal-lg {
+ height: calc(100% - 60px);
+ & > .modal-content {
+ height: 100%;
+ display: flex;
+ flex-direction: column;
+ & > .modal-body {
+ display: flex;
+ flex-direction: column;
+ flex: 1;
+ }
+ }
+ }
+}
+
+.flex-1 {
+ flex: 1;
+}
+
+.sticky-table {
+ & > div {
+ & > div:nth-child(2) {
+ & > div:first-child {
+ & > table {
+ margin-bottom: 0;
+ th {
+ border-bottom-width: 1px;
+ }
+ }
+ }
+ & > div:nth-child(2) {
+ &::-webkit-scrollbar {
+ -webkit-appearance: none;
+ &:vertical {
+ width: 7px;
+ }
+ &:horizontal {
+ height: 7px;
+ }
+ }
+
+ &::-webkit-scrollbar-thumb {
+ border-radius: 4px;
Review comment:
if these are in emotion, this can be:
`bborder-radius: ${({ theme }) => theme.gridUnit}px;`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kgabryje closed pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
kgabryje closed pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-commenter commented on pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#issuecomment-696608614
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=h1) Report
> Merging [#10982](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/448a41a4e7563cafadea1e03feb5980151e8b56d?el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `57.14%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10982 +/- ##
==========================================
- Coverage 65.77% 65.76% -0.02%
==========================================
Files 816 816
Lines 38395 38405 +10
Branches 3607 3610 +3
==========================================
+ Hits 25256 25257 +1
- Misses 13031 13040 +9
Partials 108 108
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `56.84% <16.66%> (-0.07%)` | :arrow_down: |
| #javascript | `61.72% <57.14%> (-0.01%)` | :arrow_down: |
| #python | `61.42% <ø> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...-frontend/src/datasource/ChangeDatasourceModal.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10982/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2RhdGFzb3VyY2UvQ2hhbmdlRGF0YXNvdXJjZU1vZGFsLnRzeA==) | `71.21% <57.14%> (-3.79%)` | :arrow_down: |
| [...et-frontend/src/dashboard/components/Dashboard.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10982/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0Rhc2hib2FyZC5qc3g=) | `84.70% <0.00%> (-4.71%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=footer). Last update [448a41a...0e9c075](https://codecov.io/gh/apache/incubator-superset/pull/10982?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#discussion_r497978765
##########
File path: superset-frontend/stylesheets/superset.less
##########
@@ -600,3 +600,55 @@ input[type='radio']:checked:after {
hr {
border-top: 1px solid @gray-light;
}
+
+.modal-fullscreen {
+ & > .modal-lg {
+ height: calc(100% - 60px);
+ & > .modal-content {
+ height: 100%;
+ display: flex;
+ flex-direction: column;
+ & > .modal-body {
+ display: flex;
+ flex-direction: column;
+ flex: 1;
+ }
+ }
+ }
+}
+
+.flex-1 {
+ flex: 1;
+}
+
+.sticky-table {
+ & > div {
+ & > div:nth-child(2) {
+ & > div:first-child {
+ & > table {
+ margin-bottom: 0;
+ th {
+ border-bottom-width: 1px;
+ }
+ }
+ }
+ & > div:nth-child(2) {
+ &::-webkit-scrollbar {
+ -webkit-appearance: none;
+ &:vertical {
+ width: 7px;
+ }
+ &:horizontal {
+ height: 7px;
+ }
+ }
+
+ &::-webkit-scrollbar-thumb {
+ border-radius: 4px;
+ background-color: rgba(0, 0, 0, 0.5);
+ -webkit-box-shadow: 0 0 1px rgba(255, 255, 255, 0.5);
Review comment:
```suggestion
-webkit-box-shadow: 0 0 1px ${({ theme }) => theme.colors.secondary.light5}80;
```
This should look the same, but be more theming friendly _(helooooo dark mode!)_
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kgabryje edited a comment on pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
kgabryje edited a comment on pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#issuecomment-696590695
@rusackas I did some css tweaks. They're not pretty, but necessary to override some DataTable styles
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kgabryje commented on pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
kgabryje commented on pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#issuecomment-703588882
@rusackas Due to [this](https://github.com/apache/incubator-superset/issues/11142) issue, which is related to changes made here, what do you think about postponing merging this PR until the issue is resolved? So that we don't risk another regression.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kgabryje commented on pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
kgabryje commented on pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#issuecomment-696590695
@rusackas I did some css tweaks. They're not pretty but were necessary to override some DataTable styles
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request #10982: refactor: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#discussion_r497979734
##########
File path: superset-frontend/stylesheets/superset.less
##########
@@ -600,3 +600,55 @@ input[type='radio']:checked:after {
hr {
border-top: 1px solid @gray-light;
}
+
+.modal-fullscreen {
+ & > .modal-lg {
+ height: calc(100% - 60px);
+ & > .modal-content {
+ height: 100%;
+ display: flex;
+ flex-direction: column;
+ & > .modal-body {
+ display: flex;
+ flex-direction: column;
+ flex: 1;
+ }
+ }
+ }
+}
+
+.flex-1 {
+ flex: 1;
+}
+
+.sticky-table {
+ & > div {
+ & > div:nth-child(2) {
+ & > div:first-child {
+ & > table {
+ margin-bottom: 0;
+ th {
+ border-bottom-width: 1px;
+ }
+ }
+ }
+ & > div:nth-child(2) {
+ &::-webkit-scrollbar {
+ -webkit-appearance: none;
+ &:vertical {
+ width: 7px;
Review comment:
Not sure if this 7px measurement (and the one below it) need to be exactly 7px, but if it's just _about that big_ then if this were moved to Emotion I would do `width: ${({ theme }) => theme.gridUnit * 2}px` to make it themable
```
width: 7px;
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kgabryje commented on pull request #10982: Replace reactable with DataTable from superset-ui in ChangeDatasourceModal
Posted by GitBox <gi...@apache.org>.
kgabryje commented on pull request #10982:
URL: https://github.com/apache/incubator-superset/pull/10982#issuecomment-696328506
@rusackas Can you take a look please?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org