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/07/15 16:12:47 UTC
[GitHub] [incubator-superset] nytai opened a new pull request #10298: feat(listviews): SIP-34 Bulk Select
nytai opened a new pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
- Add SIP-34 Bulk select to DatasetList DashboardList, ChartList
- convert Button.jsx -> Button.tsx (& add styling for `supersetButton`)
- refactor SubMenu to be presentation only, moving DatasetAddModal to DatasetsList
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
#### Datasets
<img width="3008" alt="Screen Shot 2020-07-12 at 10 09 06 PM" src="https://user-images.githubusercontent.com/10255196/87273357-bab3cd80-c48d-11ea-88a2-080fd8a9c32e.png">
<img width="3007" alt="Screen Shot 2020-07-12 at 10 09 21 PM" src="https://user-images.githubusercontent.com/10255196/87273383-d1f2bb00-c48d-11ea-9c5f-6f668c983900.png">
#### Charts
<img width="3008" alt="Screen Shot 2020-07-12 at 10 10 24 PM" src="https://user-images.githubusercontent.com/10255196/87273389-d6b76f00-c48d-11ea-9622-8e77086c66b4.png">
#### Dashboards
<img width="3008" alt="Screen Shot 2020-07-12 at 10 09 44 PM" src="https://user-images.githubusercontent.com/10255196/87273388-d5864200-c48d-11ea-9d23-d09798eaffa5.png">
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
- bulk select delete works for Datasets, Charts, Dashboards
- bulk expect works for Dashboards
- Dataset Add flow works
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [x] Changes UI: Behind `ENABLE_REACT_CRUD_VIEWS` config flag
- [ ] 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] nytai commented on a change in pull request #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r455197521
##########
File path: superset-frontend/src/views/dashboardList/DashboardList.tsx
##########
@@ -45,23 +45,24 @@ interface Props {
}
interface State {
- dashboards: any[];
+ bulkSelectEnabled: boolean;
dashboardCount: number;
- loading: boolean;
+ dashboards: any[];
Review comment:
DatasetList got converted into a functional component with hooks, so the shared interface makes less sense. Shared hooks on the other hand.... π€
----------------------------------------------------------------
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] etr2460 commented on a change in pull request #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r455894946
##########
File path: superset-frontend/src/components/Button.tsx
##########
@@ -17,41 +17,75 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { kebabCase } from 'lodash';
import {
Button as BootstrapButton,
Tooltip,
OverlayTrigger,
} from 'react-bootstrap';
+import styled from '@superset-ui/style';
-const propTypes = {
- children: PropTypes.node,
- className: PropTypes.string,
- tooltip: PropTypes.node,
- placement: PropTypes.string,
- onClick: PropTypes.func,
- disabled: PropTypes.bool,
- bsSize: PropTypes.string,
- bsStyle: PropTypes.string,
- btnStyles: PropTypes.string,
-};
-const defaultProps = {
- bsSize: 'sm',
- placement: 'top',
-};
+export type OnClickHandler = React.MouseEventHandler<BootstrapButton>;
+
+export interface ButtonProps {
+ className?: string;
+ tooltip?: string;
+ placement?: string;
+ onClick?: OnClickHandler;
+ disabled?: boolean;
+ bsStyle?: string;
+ btnStyles?: string;
+ bsSize?: BootstrapButton.ButtonProps['bsSize'];
+ style?: BootstrapButton.ButtonProps['style'];
+}
const BUTTON_WRAPPER_STYLE = { display: 'inline-block', cursor: 'not-allowed' };
-export default function Button(props) {
- const buttonProps = { ...props };
+const SupersetButton = styled(BootstrapButton)`
Review comment:
@nytai were you planning to action on this? Or should I review again now?
----------------------------------------------------------------
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 #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r454522216
##########
File path: superset-frontend/src/components/Button.tsx
##########
@@ -17,41 +17,70 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { kebabCase } from 'lodash';
import {
Button as BootstrapButton,
Tooltip,
OverlayTrigger,
} from 'react-bootstrap';
+import styled from '@superset-ui/style';
-const propTypes = {
- children: PropTypes.node,
- className: PropTypes.string,
- tooltip: PropTypes.node,
- placement: PropTypes.string,
- onClick: PropTypes.func,
- disabled: PropTypes.bool,
- bsSize: PropTypes.string,
- bsStyle: PropTypes.string,
- btnStyles: PropTypes.string,
-};
-const defaultProps = {
- bsSize: 'sm',
- placement: 'top',
-};
+export type OnClickHandler = React.MouseEventHandler<BootstrapButton>;
+
+export interface ButtonProps {
+ className?: string;
+ tooltip?: string;
+ placement?: string;
+ onClick?: OnClickHandler;
+ disabled?: boolean;
+ bsStyle?: string;
+ btnStyles?: string;
+ bsSize?: BootstrapButton.ButtonProps['bsSize'];
+ style?: BootstrapButton.ButtonProps['style'];
+}
const BUTTON_WRAPPER_STYLE = { display: 'inline-block', cursor: 'not-allowed' };
-export default function Button(props) {
- const buttonProps = { ...props };
+const SupersetButton = styled(BootstrapButton)`
Review comment:
π
----------------------------------------------------------------
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 #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#issuecomment-658483062
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=h1) Report
> Merging [#10298](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/2341e8d5853983f4223e7b6104b4c5bbb488a9b2&el=desc) will **increase** coverage by `0.05%`.
> The diff coverage is `86.84%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10298 +/- ##
==========================================
+ Coverage 70.60% 70.65% +0.05%
==========================================
Files 600 601 +1
Lines 32182 32269 +87
Branches 3257 3275 +18
==========================================
+ Hits 22721 22801 +80
- Misses 9358 9362 +4
- Partials 103 106 +3
```
| Flag | Coverage Ξ | |
|---|---|---|
| #cypress | `55.64% <100.00%> (+0.04%)` | :arrow_up: |
| #javascript | `59.60% <86.84%> (+0.12%)` | :arrow_up: |
| #python | `69.93% <ΓΈ> (+0.03%)` | :arrow_up: |
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/10298?src=pr&el=tree) | Coverage Ξ | |
|---|---|---|
| [...ontend/src/components/ListView/TableCollection.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvVGFibGVDb2xsZWN0aW9uLnRzeA==) | `100.00% <ΓΈ> (+6.89%)` | :arrow_up: |
| [superset-frontend/src/components/ListView/utils.ts](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvdXRpbHMudHM=) | `83.14% <ΓΈ> (ΓΈ)` | |
| [superset-frontend/src/components/Menu/Menu.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTWVudS9NZW51LmpzeA==) | `88.23% <ΓΈ> (ΓΈ)` | |
| [...frontend/src/views/datasetList/AddDatasetModal.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2RhdGFzZXRMaXN0L0FkZERhdGFzZXRNb2RhbC50c3g=) | `58.82% <0.00%> (ΓΈ)` | |
| [...uperset-frontend/src/views/chartList/ChartList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NoYXJ0TGlzdC9DaGFydExpc3QudHN4) | `60.40% <66.66%> (-0.56%)` | :arrow_down: |
| [...frontend/src/views/dashboardList/DashboardList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2Rhc2hib2FyZExpc3QvRGFzaGJvYXJkTGlzdC50c3g=) | `66.00% <75.00%> (-0.44%)` | :arrow_down: |
| [...set-frontend/src/views/datasetList/DatasetList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2RhdGFzZXRMaXN0L0RhdGFzZXRMaXN0LnRzeA==) | `71.34% <83.33%> (+2.85%)` | :arrow_up: |
| [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `68.18% <100.00%> (ΓΈ)` | |
| [superset-frontend/src/components/Button.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQnV0dG9uLnRzeA==) | `93.75% <100.00%> (ΓΈ)` | |
| [...rset-frontend/src/components/ListView/ListView.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvTGlzdFZpZXcudHN4) | `98.30% <100.00%> (+0.34%)` | :arrow_up: |
| ... and [13 more](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10298?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/10298?src=pr&el=footer). Last update [2341e8d...77e4e74](https://codecov.io/gh/apache/incubator-superset/pull/10298?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 #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r454536494
##########
File path: superset-frontend/src/components/ListView/ListView.tsx
##########
@@ -155,6 +205,37 @@ const ListView: FunctionComponent<Props> = ({
)}
</div>
<div className="body">
+ {bulkSelectEnabled && (
+ <BulkSelectWrapper bsStyle="info" onDismiss={disableBulkSelect}>
+ <div className="selectedCopy">
+ {renderBulkSelectCopy(selectedFlatRows)}
+ </div>
+ {Boolean(selectedFlatRows.length) && (
+ <>
+ <span
+ role="button"
+ tabIndex={0}
+ className="deselect-all"
+ onClick={() => toggleAllRowsSelected(false)}
+ >
+ {t('Deselect All')}
+ </span>
+ <div className="divider" />
+ {bulkActions.map(action => (
+ <Button
Review comment:
n/m the comment about the SupersetButton instance... didn't realize that's what's exported by Button
----------------------------------------------------------------
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 #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r454529485
##########
File path: superset-frontend/src/components/ListView/ListViewStyles.less
##########
@@ -104,8 +104,16 @@
}
.table-row {
+ .actions {
+ visibility: hidden;
+ }
+
&:hover {
background-color: @brand-secondary-light5;
+
+ .actions {
+ visibility: visible;
Review comment:
part II
```suggestion
opacity: 1;
```
----------------------------------------------------------------
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] nytai commented on a change in pull request #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r454554708
##########
File path: superset-frontend/src/views/dashboardList/DashboardList.tsx
##########
@@ -45,23 +45,24 @@ interface Props {
}
interface State {
- dashboards: any[];
+ bulkSelectEnabled: boolean;
dashboardCount: number;
- loading: boolean;
+ dashboards: any[];
Review comment:
that's an interesting idea.
----------------------------------------------------------------
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] nytai commented on a change in pull request #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r456086926
##########
File path: superset-frontend/src/components/ListView/ListViewStyles.less
##########
@@ -104,8 +104,16 @@
}
.table-row {
+ .actions {
+ opacity: 0;
Review comment:
Done!
----------------------------------------------------------------
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] nytai commented on a change in pull request #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r454555405
##########
File path: superset-frontend/src/components/Button.tsx
##########
@@ -17,41 +17,70 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { kebabCase } from 'lodash';
import {
Button as BootstrapButton,
Tooltip,
OverlayTrigger,
} from 'react-bootstrap';
+import styled from '@superset-ui/style';
-const propTypes = {
- children: PropTypes.node,
- className: PropTypes.string,
- tooltip: PropTypes.node,
- placement: PropTypes.string,
- onClick: PropTypes.func,
- disabled: PropTypes.bool,
- bsSize: PropTypes.string,
- bsStyle: PropTypes.string,
- btnStyles: PropTypes.string,
-};
-const defaultProps = {
- bsSize: 'sm',
- placement: 'top',
-};
+export type OnClickHandler = React.MouseEventHandler<BootstrapButton>;
+
+export interface ButtonProps {
+ className?: string;
+ tooltip?: string;
+ placement?: string;
+ onClick?: OnClickHandler;
+ disabled?: boolean;
+ bsStyle?: string;
+ btnStyles?: string;
+ bsSize?: BootstrapButton.ButtonProps['bsSize'];
+ style?: BootstrapButton.ButtonProps['style'];
+}
const BUTTON_WRAPPER_STYLE = { display: 'inline-block', cursor: 'not-allowed' };
-export default function Button(props) {
- const buttonProps = { ...props };
+const SupersetButton = styled(BootstrapButton)`
+ &.supersetButton {
Review comment:
This button is used in other areas of the product, so there's potential for breakage. Adding the `.supersetButton` class renders the button as a wide CTA button (as per the SIP-34 designs). We could abstract this into a prop and use that instead, something like `style: 'bootstrap' | 'default' | 'small' | 'link' | ...` to control these styles, where `bootstrap` defaults to the bootstrap styles, and the others pertain to the new styling.
----------------------------------------------------------------
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 #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r456058050
##########
File path: superset-frontend/src/components/ListView/ListViewStyles.less
##########
@@ -104,8 +104,16 @@
}
.table-row {
+ .actions {
+ opacity: 0;
Review comment:
Awwww, didn't like the animation?
`transition: opacity @timing-normal;`
^I'm tempted to add this kind of thing lots of places in Superset ;)
----------------------------------------------------------------
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 #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r454519472
##########
File path: superset-frontend/spec/javascripts/components/ListView/ListView_spec.jsx
##########
@@ -72,8 +73,15 @@ const mockedProps = {
pageSize: 1,
fetchData: jest.fn(() => []),
loading: false,
+ bulkSelectEnabled: true,
Review comment:
Maybe worth adding a sanity check test where `bulkSelectEnabled: false` to make sure it loads correctly and the buttons _aren't_ in the DOM? Β―\\\_(γ)_/Β―
----------------------------------------------------------------
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 #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r454519472
##########
File path: superset-frontend/spec/javascripts/components/ListView/ListView_spec.jsx
##########
@@ -72,8 +73,15 @@ const mockedProps = {
pageSize: 1,
fetchData: jest.fn(() => []),
loading: false,
+ bulkSelectEnabled: true,
Review comment:
Maybe worth adding a sanity check test where `bulkSelectEnabled: false` to make sure it loads correctly and the buttons _aren't_ in the DOM? Β―\_(γ)_/Β―
----------------------------------------------------------------
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 #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r454524686
##########
File path: superset-frontend/src/components/Button.tsx
##########
@@ -17,41 +17,70 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { kebabCase } from 'lodash';
import {
Button as BootstrapButton,
Tooltip,
OverlayTrigger,
} from 'react-bootstrap';
+import styled from '@superset-ui/style';
-const propTypes = {
- children: PropTypes.node,
- className: PropTypes.string,
- tooltip: PropTypes.node,
- placement: PropTypes.string,
- onClick: PropTypes.func,
- disabled: PropTypes.bool,
- bsSize: PropTypes.string,
- bsStyle: PropTypes.string,
- btnStyles: PropTypes.string,
-};
-const defaultProps = {
- bsSize: 'sm',
- placement: 'top',
-};
+export type OnClickHandler = React.MouseEventHandler<BootstrapButton>;
+
+export interface ButtonProps {
+ className?: string;
+ tooltip?: string;
+ placement?: string;
+ onClick?: OnClickHandler;
+ disabled?: boolean;
+ bsStyle?: string;
+ btnStyles?: string;
+ bsSize?: BootstrapButton.ButtonProps['bsSize'];
+ style?: BootstrapButton.ButtonProps['style'];
+}
const BUTTON_WRAPPER_STYLE = { display: 'inline-block', cursor: 'not-allowed' };
-export default function Button(props) {
- const buttonProps = { ...props };
+const SupersetButton = styled(BootstrapButton)`
+ &.supersetButton {
Review comment:
Think we need to have the .supersetButton class here anymore, or should these be the default styles for SupersetButton?
----------------------------------------------------------------
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 #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r454527282
##########
File path: superset-frontend/src/components/ListView/ListView.tsx
##########
@@ -155,6 +205,37 @@ const ListView: FunctionComponent<Props> = ({
)}
</div>
<div className="body">
+ {bulkSelectEnabled && (
+ <BulkSelectWrapper bsStyle="info" onDismiss={disableBulkSelect}>
+ <div className="selectedCopy">
+ {renderBulkSelectCopy(selectedFlatRows)}
+ </div>
+ {Boolean(selectedFlatRows.length) && (
+ <>
+ <span
+ role="button"
+ tabIndex={0}
+ className="deselect-all"
+ onClick={() => toggleAllRowsSelected(false)}
+ >
+ {t('Deselect All')}
+ </span>
+ <div className="divider" />
+ {bulkActions.map(action => (
+ <Button
Review comment:
Should this be an instance of SupersetButton?
Also, I'm assuming the margin-left style applied is to add a gap between buttons. If so, maybe we can kill off the margin-left style in this CSS, and add it to the SupersetButton component, e.g.:
```
margin-left: ${({ theme }) => theme.gridUnit * 4}px;
&:first-of-type {
margin-left: 0;
}
```
----------------------------------------------------------------
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 #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#issuecomment-658483062
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=h1) Report
> Merging [#10298](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/7abe3e518298ec896b60e28a8de63e444053e69b&el=desc) will **increase** coverage by `0.07%`.
> The diff coverage is `85.71%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10298 +/- ##
==========================================
+ Coverage 70.58% 70.66% +0.07%
==========================================
Files 600 600
Lines 32093 32189 +96
Branches 3244 3273 +29
==========================================
+ Hits 22654 22745 +91
- Misses 9335 9338 +3
- Partials 104 106 +2
```
| Flag | Coverage Ξ | |
|---|---|---|
| #cypress | `55.63% <100.00%> (+0.15%)` | :arrow_up: |
| #javascript | `59.67% <85.71%> (+0.09%)` | :arrow_up: |
| #python | `69.86% <ΓΈ> (+0.07%)` | :arrow_up: |
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=tree) | Coverage Ξ | |
|---|---|---|
| [...ontend/src/components/ListView/TableCollection.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvVGFibGVDb2xsZWN0aW9uLnRzeA==) | `100.00% <ΓΈ> (+6.89%)` | :arrow_up: |
| [superset-frontend/src/components/ListView/utils.ts](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvdXRpbHMudHM=) | `83.14% <ΓΈ> (ΓΈ)` | |
| [superset-frontend/src/components/Menu/Menu.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTWVudS9NZW51LmpzeA==) | `88.23% <ΓΈ> (ΓΈ)` | |
| [...frontend/src/views/datasetList/AddDatasetModal.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2RhdGFzZXRMaXN0L0FkZERhdGFzZXRNb2RhbC50c3g=) | `58.82% <0.00%> (ΓΈ)` | |
| [...uperset-frontend/src/views/chartList/ChartList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NoYXJ0TGlzdC9DaGFydExpc3QudHN4) | `60.66% <66.66%> (-0.56%)` | :arrow_down: |
| [...frontend/src/views/dashboardList/DashboardList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2Rhc2hib2FyZExpc3QvRGFzaGJvYXJkTGlzdC50c3g=) | `66.22% <75.00%> (-0.45%)` | :arrow_down: |
| [...set-frontend/src/views/datasetList/DatasetList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2RhdGFzZXRMaXN0L0RhdGFzZXRMaXN0LnRzeA==) | `71.17% <82.92%> (+2.68%)` | :arrow_up: |
| [...rset-frontend/src/components/ListView/ListView.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvTGlzdFZpZXcudHN4) | `96.55% <94.11%> (-1.41%)` | :arrow_down: |
| [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `68.18% <100.00%> (ΓΈ)` | |
| [superset-frontend/src/components/Button.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQnV0dG9uLnRzeA==) | `93.33% <100.00%> (ΓΈ)` | |
| ... and [50 more](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10298?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/10298?src=pr&el=footer). Last update [7abe3e5...3496599](https://codecov.io/gh/apache/incubator-superset/pull/10298?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] codecov-commenter edited a comment on pull request #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#issuecomment-658483062
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=h1) Report
> Merging [#10298](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/7abe3e518298ec896b60e28a8de63e444053e69b&el=desc) will **increase** coverage by `0.06%`.
> The diff coverage is `88.23%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10298 +/- ##
==========================================
+ Coverage 70.58% 70.65% +0.06%
==========================================
Files 600 600
Lines 32093 32185 +92
Branches 3244 3257 +13
==========================================
+ Hits 22654 22740 +86
- Misses 9335 9344 +9
+ Partials 104 101 -3
```
| Flag | Coverage Ξ | |
|---|---|---|
| #cypress | `55.84% <71.05%> (+0.37%)` | :arrow_up: |
| #javascript | `59.47% <86.30%> (-0.10%)` | :arrow_down: |
| #python | `69.86% <91.66%> (+0.07%)` | :arrow_up: |
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=tree) | Coverage Ξ | |
|---|---|---|
| [...erset-frontend/src/SqlLab/components/ResultSet.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC50c3g=) | `80.91% <ΓΈ> (ΓΈ)` | |
| [...tend/src/SqlLab/components/ScheduleQueryButton.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NjaGVkdWxlUXVlcnlCdXR0b24uanN4) | `8.62% <ΓΈ> (ΓΈ)` | |
| [superset-frontend/src/SqlLab/constants.ts](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb25zdGFudHMudHM=) | `100.00% <ΓΈ> (ΓΈ)` | |
| [superset-frontend/src/SqlLab/index.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9pbmRleC50c3g=) | `25.00% <ΓΈ> (ΓΈ)` | |
| [superset-frontend/src/chart/Chart.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0LmpzeA==) | `65.30% <0.00%> (-1.37%)` | :arrow_down: |
| [...src/dashboard/components/HeaderActionsDropdown.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0hlYWRlckFjdGlvbnNEcm9wZG93bi5qc3g=) | `79.16% <ΓΈ> (ΓΈ)` | |
| [...d/src/dashboard/components/SliceHeaderControls.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL1NsaWNlSGVhZGVyQ29udHJvbHMuanN4) | `65.95% <ΓΈ> (ΓΈ)` | |
| [.../src/explore/components/ControlPanelsContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9Db250cm9sUGFuZWxzQ29udGFpbmVyLmpzeA==) | `92.30% <ΓΈ> (ΓΈ)` | |
| [...nd/src/explore/components/ExploreActionButtons.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQWN0aW9uQnV0dG9ucy5qc3g=) | `100.00% <ΓΈ> (ΓΈ)` | |
| [...tend/src/explore/components/ExploreChartHeader.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQ2hhcnRIZWFkZXIuanN4) | `82.35% <ΓΈ> (ΓΈ)` | |
| ... and [53 more](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10298?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/10298?src=pr&el=footer). Last update [7abe3e5...b35adea](https://codecov.io/gh/apache/incubator-superset/pull/10298?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] etr2460 commented on a change in pull request #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r456110133
##########
File path: superset-frontend/src/components/Button.tsx
##########
@@ -17,41 +17,75 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { kebabCase } from 'lodash';
import {
Button as BootstrapButton,
Tooltip,
OverlayTrigger,
} from 'react-bootstrap';
+import styled from '@superset-ui/style';
-const propTypes = {
- children: PropTypes.node,
- className: PropTypes.string,
- tooltip: PropTypes.node,
- placement: PropTypes.string,
- onClick: PropTypes.func,
- disabled: PropTypes.bool,
- bsSize: PropTypes.string,
- bsStyle: PropTypes.string,
- btnStyles: PropTypes.string,
-};
-const defaultProps = {
- bsSize: 'sm',
- placement: 'top',
-};
+export type OnClickHandler = React.MouseEventHandler<BootstrapButton>;
+
+export interface ButtonProps {
+ className?: string;
+ tooltip?: string;
+ placement?: string;
+ onClick?: OnClickHandler;
+ disabled?: boolean;
+ bsStyle?: string;
+ btnStyles?: string;
+ bsSize?: BootstrapButton.ButtonProps['bsSize'];
+ style?: BootstrapButton.ButtonProps['style'];
+}
const BUTTON_WRAPPER_STYLE = { display: 'inline-block', cursor: 'not-allowed' };
-export default function Button(props) {
- const buttonProps = { ...props };
+const SupersetButton = styled(BootstrapButton)`
+ &.supersetButton {
+ border-radius: ${({ theme }) => theme.gridUnit}px;
+ border: none;
+ color: ${({ theme }) => theme.colors.secondary.light5};
+ font-size: ${({ theme }) => theme.typography.sizes.s};
+ font-weight: ${({ theme }) => theme.typography.weights.bold};
+ min-width: 144px;
+ min-height: 32px;
Review comment:
do you think a factor of the gridUnit should be used here too?
##########
File path: superset-frontend/src/components/Button.tsx
##########
@@ -17,41 +17,75 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { kebabCase } from 'lodash';
import {
Button as BootstrapButton,
Tooltip,
OverlayTrigger,
} from 'react-bootstrap';
+import styled from '@superset-ui/style';
-const propTypes = {
- children: PropTypes.node,
- className: PropTypes.string,
- tooltip: PropTypes.node,
- placement: PropTypes.string,
- onClick: PropTypes.func,
- disabled: PropTypes.bool,
- bsSize: PropTypes.string,
- bsStyle: PropTypes.string,
- btnStyles: PropTypes.string,
-};
-const defaultProps = {
- bsSize: 'sm',
- placement: 'top',
-};
+export type OnClickHandler = React.MouseEventHandler<BootstrapButton>;
+
+export interface ButtonProps {
+ className?: string;
+ tooltip?: string;
+ placement?: string;
+ onClick?: OnClickHandler;
+ disabled?: boolean;
+ bsStyle?: string;
+ btnStyles?: string;
+ bsSize?: BootstrapButton.ButtonProps['bsSize'];
+ style?: BootstrapButton.ButtonProps['style'];
+}
const BUTTON_WRAPPER_STYLE = { display: 'inline-block', cursor: 'not-allowed' };
-export default function Button(props) {
- const buttonProps = { ...props };
+const SupersetButton = styled(BootstrapButton)`
+ &.supersetButton {
+ border-radius: ${({ theme }) => theme.gridUnit}px;
Review comment:
should this use `theme.borderRadius`? I think they're both set to 4
##########
File path: superset-frontend/src/components/Button.tsx
##########
@@ -17,41 +17,75 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { kebabCase } from 'lodash';
import {
Button as BootstrapButton,
Tooltip,
OverlayTrigger,
} from 'react-bootstrap';
+import styled from '@superset-ui/style';
-const propTypes = {
- children: PropTypes.node,
- className: PropTypes.string,
- tooltip: PropTypes.node,
- placement: PropTypes.string,
- onClick: PropTypes.func,
- disabled: PropTypes.bool,
- bsSize: PropTypes.string,
- bsStyle: PropTypes.string,
- btnStyles: PropTypes.string,
-};
-const defaultProps = {
- bsSize: 'sm',
- placement: 'top',
-};
+export type OnClickHandler = React.MouseEventHandler<BootstrapButton>;
+
+export interface ButtonProps {
+ className?: string;
+ tooltip?: string;
+ placement?: string;
+ onClick?: OnClickHandler;
+ disabled?: boolean;
+ bsStyle?: string;
+ btnStyles?: string;
+ bsSize?: BootstrapButton.ButtonProps['bsSize'];
+ style?: BootstrapButton.ButtonProps['style'];
+}
const BUTTON_WRAPPER_STYLE = { display: 'inline-block', cursor: 'not-allowed' };
-export default function Button(props) {
- const buttonProps = { ...props };
+const SupersetButton = styled(BootstrapButton)`
+ &.supersetButton {
+ border-radius: ${({ theme }) => theme.gridUnit}px;
+ border: none;
+ color: ${({ theme }) => theme.colors.secondary.light5};
+ font-size: ${({ theme }) => theme.typography.sizes.s};
+ font-weight: ${({ theme }) => theme.typography.weights.bold};
+ min-width: 144px;
+ min-height: 32px;
+ text-transform: uppercase;
+ margin-left: ${({ theme }) => theme.gridUnit * 4}px;
+ &:first-of-type {
+ margin-left: 0;
+ }
+
+ i {
+ padding: 0 ${({ theme }) => theme.gridUnit * 2}px 0 0;
+ }
+
+ &.primary {
+ background-color: ${({ theme }) => theme.colors.primary.base};
+ }
+ &.secondary {
+ color: ${({ theme }) => theme.colors.primary.base};
+ background-color: ${({ theme }) => theme.colors.primary.light4};
+ }
+ &.danger {
+ background-color: ${({ theme }) => theme.colors.error.base};
+ }
+ }
+`;
+
+const Button: React.FunctionComponent<ButtonProps> = props => {
Review comment:
instead of using arrow functions like this, I think it's better to do:
```tsx
export default function Button(props: ButtonProps) {
...
```
The arrow function results in an anonymous function and makes stack traces harder to read
----------------------------------------------------------------
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 #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r454545481
##########
File path: superset-frontend/src/views/datasetList/DatasetList.tsx
##########
@@ -532,50 +567,64 @@ const DatasetList: FunctionComponent<DatasetListProps> = ({
onConfirm={handleBulkDatasetDelete}
>
{confirmDelete => {
- const bulkActions = [];
- if (canDelete()) {
- bulkActions.push({
- key: 'delete',
- name: (
- <>
- <i className="fa fa-trash" /> {t('Delete')}
- </>
- ),
- onSelect: confirmDelete,
- });
- }
+ const bulkActions: ListViewProps['bulkActions'] = canDelete
+ ? [
+ {
+ key: 'delete',
+ name: t('Delete'),
+ onSelect: confirmDelete,
+ type: 'danger',
+ },
+ ]
+ : [];
+
return (
- <>
- {datasetCurrentlyDeleting && (
- <DeleteModal
- description={t(
- `The dataset ${datasetCurrentlyDeleting.table_name} is linked to
- ${datasetCurrentlyDeleting.chart_count} charts that appear on
- ${datasetCurrentlyDeleting.dashboard_count} dashboards.
- Are you sure you want to continue? Deleting the dataset will break
- those objects.`,
- )}
- onConfirm={() =>
- handleDatasetDelete(datasetCurrentlyDeleting)
- }
- onHide={closeDatasetDeleteModal}
- open
- title={t('Delete Dataset?')}
- />
- )}
- <ListView
- className="dataset-list-view"
- columns={columns}
- data={datasets}
- count={datasetCount}
- pageSize={PAGE_SIZE}
- fetchData={fetchData}
- loading={loading}
- initialSort={initialSort}
- filters={currentFilters}
- bulkActions={bulkActions}
- />
- </>
+ <ListView
+ className="dataset-list-view"
+ columns={columns}
+ data={datasets}
+ count={datasetCount}
+ pageSize={PAGE_SIZE}
+ fetchData={fetchData}
+ loading={loading}
+ initialSort={initialSort}
+ filters={currentFilters}
+ bulkActions={bulkActions}
+ bulkSelectEnabled={bulkSelectEnabled}
+ disableBulkSelect={() => setBulkSelectEnabled(false)}
+ renderBulkSelectCopy={selected => {
+ const { virtualCount, physicalCount } = selected.reduce(
+ (acc, e) => {
+ if (e.original.kind === 'physical') acc.physicalCount += 1;
+ else if (e.original.kind === 'virtual')
+ acc.virtualCount += 1;
+ return acc;
+ },
+ { virtualCount: 0, physicalCount: 0 },
+ );
+
+ if (!selected.length) return t('0 Selected');
+ else if (virtualCount && !physicalCount)
+ return t(
+ '%s Selected (Virtual)',
+ selected.length,
+ virtualCount,
+ );
+ else if (physicalCount && !virtualCount)
+ return t(
+ '%s Selected (Physical)',
+ selected.length,
+ physicalCount,
+ );
+
+ return t(
+ '%s Selected (%s Physical, %s Virtual)',
Review comment:
These strings seem like a great expectation for a unit test that pokes around making various combinations of bulk selection controls.
----------------------------------------------------------------
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] nytai closed pull request #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
nytai closed pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298
----------------------------------------------------------------
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] nytai commented on a change in pull request #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r454557461
##########
File path: superset-frontend/src/components/ListView/ListView.tsx
##########
@@ -155,6 +205,37 @@ const ListView: FunctionComponent<Props> = ({
)}
</div>
<div className="body">
+ {bulkSelectEnabled && (
+ <BulkSelectWrapper bsStyle="info" onDismiss={disableBulkSelect}>
+ <div className="selectedCopy">
+ {renderBulkSelectCopy(selectedFlatRows)}
+ </div>
+ {Boolean(selectedFlatRows.length) && (
+ <>
+ <span
+ role="button"
+ tabIndex={0}
+ className="deselect-all"
+ onClick={() => toggleAllRowsSelected(false)}
+ >
+ {t('Deselect All')}
+ </span>
+ <div className="divider" />
+ {bulkActions.map(action => (
+ <Button
Review comment:
I'll give that styling a shot, originally I has the margins on the actual button component, but it seemed like there's more flexibility in the button not assuming anything about it's surrounding.
----------------------------------------------------------------
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 #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#issuecomment-658483062
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=h1) Report
> Merging [#10298](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/2341e8d5853983f4223e7b6104b4c5bbb488a9b2&el=desc) will **decrease** coverage by `4.92%`.
> The diff coverage is `86.84%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10298 +/- ##
==========================================
- Coverage 70.60% 65.67% -4.93%
==========================================
Files 600 601 +1
Lines 32182 32268 +86
Branches 3257 3275 +18
==========================================
- Hits 22721 21191 -1530
- Misses 9358 10893 +1535
- Partials 103 184 +81
```
| Flag | Coverage Ξ | |
|---|---|---|
| #cypress | `?` | |
| #javascript | `59.60% <86.84%> (+0.12%)` | :arrow_up: |
| #python | `69.93% <ΓΈ> (+0.03%)` | :arrow_up: |
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/10298?src=pr&el=tree) | Coverage Ξ | |
|---|---|---|
| [...ontend/src/components/ListView/TableCollection.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvVGFibGVDb2xsZWN0aW9uLnRzeA==) | `100.00% <ΓΈ> (+6.89%)` | :arrow_up: |
| [superset-frontend/src/components/ListView/utils.ts](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvdXRpbHMudHM=) | `83.14% <ΓΈ> (ΓΈ)` | |
| [superset-frontend/src/components/Menu/Menu.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTWVudS9NZW51LmpzeA==) | `88.23% <ΓΈ> (ΓΈ)` | |
| [...frontend/src/views/datasetList/AddDatasetModal.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2RhdGFzZXRMaXN0L0FkZERhdGFzZXRNb2RhbC50c3g=) | `58.82% <0.00%> (ΓΈ)` | |
| [...uperset-frontend/src/views/chartList/ChartList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NoYXJ0TGlzdC9DaGFydExpc3QudHN4) | `60.40% <66.66%> (-0.56%)` | :arrow_down: |
| [...frontend/src/views/dashboardList/DashboardList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2Rhc2hib2FyZExpc3QvRGFzaGJvYXJkTGlzdC50c3g=) | `66.00% <75.00%> (-0.44%)` | :arrow_down: |
| [...set-frontend/src/views/datasetList/DatasetList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2RhdGFzZXRMaXN0L0RhdGFzZXRMaXN0LnRzeA==) | `71.34% <83.33%> (+2.85%)` | :arrow_up: |
| [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `68.18% <100.00%> (ΓΈ)` | |
| [superset-frontend/src/components/Button.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQnV0dG9uLnRzeA==) | `93.75% <100.00%> (ΓΈ)` | |
| [...rset-frontend/src/components/ListView/ListView.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvTGlzdFZpZXcudHN4) | `98.30% <100.00%> (+0.34%)` | :arrow_up: |
| ... and [165 more](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10298?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/10298?src=pr&el=footer). Last update [2341e8d...77e4e74](https://codecov.io/gh/apache/incubator-superset/pull/10298?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] nytai commented on a change in pull request #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r455170161
##########
File path: superset-frontend/src/views/datasetList/DatasetList.tsx
##########
@@ -532,50 +567,64 @@ const DatasetList: FunctionComponent<DatasetListProps> = ({
onConfirm={handleBulkDatasetDelete}
>
{confirmDelete => {
- const bulkActions = [];
- if (canDelete()) {
- bulkActions.push({
- key: 'delete',
- name: (
- <>
- <i className="fa fa-trash" /> {t('Delete')}
- </>
- ),
- onSelect: confirmDelete,
- });
- }
+ const bulkActions: ListViewProps['bulkActions'] = canDelete
+ ? [
+ {
+ key: 'delete',
+ name: t('Delete'),
+ onSelect: confirmDelete,
+ type: 'danger',
+ },
+ ]
+ : [];
+
return (
- <>
- {datasetCurrentlyDeleting && (
- <DeleteModal
- description={t(
- `The dataset ${datasetCurrentlyDeleting.table_name} is linked to
- ${datasetCurrentlyDeleting.chart_count} charts that appear on
- ${datasetCurrentlyDeleting.dashboard_count} dashboards.
- Are you sure you want to continue? Deleting the dataset will break
- those objects.`,
- )}
- onConfirm={() =>
- handleDatasetDelete(datasetCurrentlyDeleting)
- }
- onHide={closeDatasetDeleteModal}
- open
- title={t('Delete Dataset?')}
- />
- )}
- <ListView
- className="dataset-list-view"
- columns={columns}
- data={datasets}
- count={datasetCount}
- pageSize={PAGE_SIZE}
- fetchData={fetchData}
- loading={loading}
- initialSort={initialSort}
- filters={currentFilters}
- bulkActions={bulkActions}
- />
- </>
+ <ListView
+ className="dataset-list-view"
+ columns={columns}
+ data={datasets}
+ count={datasetCount}
+ pageSize={PAGE_SIZE}
+ fetchData={fetchData}
+ loading={loading}
+ initialSort={initialSort}
+ filters={currentFilters}
+ bulkActions={bulkActions}
+ bulkSelectEnabled={bulkSelectEnabled}
+ disableBulkSelect={() => setBulkSelectEnabled(false)}
+ renderBulkSelectCopy={selected => {
+ const { virtualCount, physicalCount } = selected.reduce(
+ (acc, e) => {
+ if (e.original.kind === 'physical') acc.physicalCount += 1;
+ else if (e.original.kind === 'virtual')
+ acc.virtualCount += 1;
+ return acc;
+ },
+ { virtualCount: 0, physicalCount: 0 },
+ );
+
+ if (!selected.length) return t('0 Selected');
+ else if (virtualCount && !physicalCount)
+ return t(
+ '%s Selected (Virtual)',
+ selected.length,
+ virtualCount,
+ );
+ else if (physicalCount && !virtualCount)
+ return t(
+ '%s Selected (Physical)',
+ selected.length,
+ physicalCount,
+ );
Review comment:
I agree with this, I'd be in favor of turning https://eslint.org/docs/rules/curly on
----------------------------------------------------------------
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] nytai commented on a change in pull request #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r455977377
##########
File path: superset-frontend/src/components/Button.tsx
##########
@@ -17,41 +17,75 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { kebabCase } from 'lodash';
import {
Button as BootstrapButton,
Tooltip,
OverlayTrigger,
} from 'react-bootstrap';
+import styled from '@superset-ui/style';
-const propTypes = {
- children: PropTypes.node,
- className: PropTypes.string,
- tooltip: PropTypes.node,
- placement: PropTypes.string,
- onClick: PropTypes.func,
- disabled: PropTypes.bool,
- bsSize: PropTypes.string,
- bsStyle: PropTypes.string,
- btnStyles: PropTypes.string,
-};
-const defaultProps = {
- bsSize: 'sm',
- placement: 'top',
-};
+export type OnClickHandler = React.MouseEventHandler<BootstrapButton>;
+
+export interface ButtonProps {
+ className?: string;
+ tooltip?: string;
+ placement?: string;
+ onClick?: OnClickHandler;
+ disabled?: boolean;
+ bsStyle?: string;
+ btnStyles?: string;
+ bsSize?: BootstrapButton.ButtonProps['bsSize'];
+ style?: BootstrapButton.ButtonProps['style'];
+}
const BUTTON_WRAPPER_STYLE = { display: 'inline-block', cursor: 'not-allowed' };
-export default function Button(props) {
- const buttonProps = { ...props };
+const SupersetButton = styled(BootstrapButton)`
Review comment:
@etr2460 Not planning on actioning this now, mind reviewing this as is? The fix for testing hooks is needed for other PRs. I'll consider addressing this when rebasing https://github.com/apache/incubator-superset/pull/10335 after this merges.
----------------------------------------------------------------
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 #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r455985272
##########
File path: superset-frontend/src/components/Button.tsx
##########
@@ -17,41 +17,70 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { kebabCase } from 'lodash';
import {
Button as BootstrapButton,
Tooltip,
OverlayTrigger,
} from 'react-bootstrap';
+import styled from '@superset-ui/style';
-const propTypes = {
- children: PropTypes.node,
- className: PropTypes.string,
- tooltip: PropTypes.node,
- placement: PropTypes.string,
- onClick: PropTypes.func,
- disabled: PropTypes.bool,
- bsSize: PropTypes.string,
- bsStyle: PropTypes.string,
- btnStyles: PropTypes.string,
-};
-const defaultProps = {
- bsSize: 'sm',
- placement: 'top',
-};
+export type OnClickHandler = React.MouseEventHandler<BootstrapButton>;
+
+export interface ButtonProps {
+ className?: string;
+ tooltip?: string;
+ placement?: string;
+ onClick?: OnClickHandler;
+ disabled?: boolean;
+ bsStyle?: string;
+ btnStyles?: string;
+ bsSize?: BootstrapButton.ButtonProps['bsSize'];
+ style?: BootstrapButton.ButtonProps['style'];
+}
const BUTTON_WRAPPER_STYLE = { display: 'inline-block', cursor: 'not-allowed' };
-export default function Button(props) {
- const buttonProps = { ...props };
+const SupersetButton = styled(BootstrapButton)`
+ &.supersetButton {
Review comment:
I like that idea... then ALL the buttons usage can be migrated to import/use the new Button component, instead of direct (React-)Bootstrap use, and they'll be easy to find/replace as we go forward with SIP-34.
##########
File path: superset-frontend/src/components/ListView/ListView.tsx
##########
@@ -155,6 +205,37 @@ const ListView: FunctionComponent<Props> = ({
)}
</div>
<div className="body">
+ {bulkSelectEnabled && (
+ <BulkSelectWrapper bsStyle="info" onDismiss={disableBulkSelect}>
+ <div className="selectedCopy">
+ {renderBulkSelectCopy(selectedFlatRows)}
+ </div>
+ {Boolean(selectedFlatRows.length) && (
+ <>
+ <span
+ role="button"
+ tabIndex={0}
+ className="deselect-all"
+ onClick={() => toggleAllRowsSelected(false)}
+ >
+ {t('Deselect All')}
+ </span>
+ <div className="divider" />
+ {bulkActions.map(action => (
+ <Button
Review comment:
Probably debatable, but my gut tells me that one component serving both makes sense, perhaps with some props to drive it as you've suggested. Then all the code is in one spot, and easier to maintain/migrate/style, especially if/when we shift into AntD.
----------------------------------------------------------------
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] etr2460 commented on a change in pull request #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r454785527
##########
File path: superset-frontend/src/components/ListView/ListView.tsx
##########
@@ -42,12 +44,52 @@ interface Props {
filters?: Filters;
bulkActions?: Array<{
key: string;
- name: React.ReactNode | string;
+ name: React.ReactNode;
onSelect: (rows: any[]) => any;
+ type?: 'primary' | 'secondary' | 'danger';
}>;
isSIP34FilterUIEnabled?: boolean;
+ bulkSelectEnabled?: boolean;
+ disableBulkSelect?: () => void;
+ renderBulkSelectCopy?: (selects: any[]) => React.ReactNode;
}
+const BulkSelectWrapper = styled(Alert)`
+ border-radius: 0;
+ margin-bottom: 0;
+ padding-top: 0;
+ padding-bottom: 0;
+ padding-right: 16px;
+ padding-right: 36px;
+ color: #3d3d3d;
+ background-color: ${({ theme }) => theme.colors.primary.light4};
+
+ .selectedCopy {
+ display: inline-block;
+ padding: 16px 0;
+ }
+
+ .deselect-all {
+ color: #1985a0;
+ margin-left: 16px;
+ }
+
+ .divider {
+ margin: -8px 0 -8px 16px;
+ width: 1px;
+ height: 32px;
+ background: rgba(0, 0, 0, 0.0001);
Review comment:
Is this color even visible?
##########
File path: superset-frontend/src/components/Button.tsx
##########
@@ -17,41 +17,75 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { kebabCase } from 'lodash';
import {
Button as BootstrapButton,
Tooltip,
OverlayTrigger,
} from 'react-bootstrap';
+import styled from '@superset-ui/style';
-const propTypes = {
- children: PropTypes.node,
- className: PropTypes.string,
- tooltip: PropTypes.node,
- placement: PropTypes.string,
- onClick: PropTypes.func,
- disabled: PropTypes.bool,
- bsSize: PropTypes.string,
- bsStyle: PropTypes.string,
- btnStyles: PropTypes.string,
-};
-const defaultProps = {
- bsSize: 'sm',
- placement: 'top',
-};
+export type OnClickHandler = React.MouseEventHandler<BootstrapButton>;
+
+export interface ButtonProps {
+ className?: string;
+ tooltip?: string;
+ placement?: string;
+ onClick?: OnClickHandler;
+ disabled?: boolean;
+ bsStyle?: string;
+ btnStyles?: string;
+ bsSize?: BootstrapButton.ButtonProps['bsSize'];
+ style?: BootstrapButton.ButtonProps['style'];
+}
const BUTTON_WRAPPER_STYLE = { display: 'inline-block', cursor: 'not-allowed' };
-export default function Button(props) {
- const buttonProps = { ...props };
+const SupersetButton = styled(BootstrapButton)`
Review comment:
How is this styling different from `/views/datasetList/Button`? Should these two components be joined together?
cc @lilykuang
##########
File path: superset-frontend/src/components/ListView/ListView.tsx
##########
@@ -155,6 +201,43 @@ const ListView: FunctionComponent<Props> = ({
)}
</div>
<div className="body">
+ {bulkSelectEnabled && (
+ <BulkSelectWrapper
+ data-test="bulk-select-controls"
+ bsStyle="info"
+ onDismiss={disableBulkSelect}
+ >
+ <div className="selectedCopy" data-test="bulk-select-copy">
+ {renderBulkSelectCopy(selectedFlatRows)}
+ </div>
+ {Boolean(selectedFlatRows.length) && (
+ <>
+ <span
+ data-test="bulk-select-deselect-all"
+ role="button"
+ tabIndex={0}
+ className="deselect-all"
+ onClick={() => toggleAllRowsSelected(false)}
+ >
+ {t('Deselect All')}
+ </span>
+ <div className="divider" />
+ {bulkActions.map(action => (
+ <Button
+ data-test="bulk-select-action"
+ key={action.key}
+ className={`supersetButton ${action.type}`}
Review comment:
should this use `cx`?
##########
File path: superset-frontend/src/components/ListView/ListView.tsx
##########
@@ -42,12 +44,52 @@ interface Props {
filters?: Filters;
bulkActions?: Array<{
key: string;
- name: React.ReactNode | string;
+ name: React.ReactNode;
onSelect: (rows: any[]) => any;
+ type?: 'primary' | 'secondary' | 'danger';
}>;
isSIP34FilterUIEnabled?: boolean;
+ bulkSelectEnabled?: boolean;
+ disableBulkSelect?: () => void;
+ renderBulkSelectCopy?: (selects: any[]) => React.ReactNode;
}
+const BulkSelectWrapper = styled(Alert)`
+ border-radius: 0;
+ margin-bottom: 0;
+ padding-top: 0;
+ padding-bottom: 0;
+ padding-right: 16px;
+ padding-right: 36px;
Review comment:
probably only one of these should exist
##########
File path: superset-frontend/src/components/Menu/SubMenu.tsx
##########
@@ -16,40 +16,29 @@
* specific language governing permissions and limitations
* under the License.
*/
-import React, { useState } from 'react';
+import React from 'react';
import styled from '@superset-ui/style';
-import DatasetModal from 'src/views/datasetList/DatasetModal';
-import { Button, Nav, Navbar, MenuItem } from 'react-bootstrap';
+import { Nav, Navbar, MenuItem } from 'react-bootstrap';
+import Button, { OnClickHandler } from 'src/components/Button';
const StyledHeader = styled.header`
margin-top: -20px;
.navbar-header .navbar-brand {
font-weight: ${({ theme }) => theme.typography.weights.bold};
}
-
.navbar-right {
- .btn-default {
- background-color: ${({ theme }) => theme.colors.primary.base};
- border-radius: 4px;
- border: none;
- color: ${({ theme }) => theme.colors.secondary.light5};
- font-size: ${({ theme }) => theme.typography.sizes.s};
- font-weight: ${({ theme }) => theme.typography.weights.bold};
- margin: 8px 43px;
- padding: 8px 51px 8px 43px;
- text-transform: uppercase;
- i {
- padding: 4px ${({ theme }) => theme.typography.sizes.xs};
- }
+ .supersetButton {
+ margin: ${({ theme }) => theme.gridUnit * 2}px
+ ${({ theme }) => theme.gridUnit * 4}px
+ ${({ theme }) => theme.gridUnit * 2}px 0;
Review comment:
this is kinda tough to read... maybe this is easier?:
```
${({theme}) => `${theme.gridUnit * 2}px ${theme.gridUnit * 4}px ${theme.gridUnit * 2}px 0`}
```
##########
File path: superset-frontend/src/views/datasetList/DatasetList.tsx
##########
@@ -532,50 +567,64 @@ const DatasetList: FunctionComponent<DatasetListProps> = ({
onConfirm={handleBulkDatasetDelete}
>
{confirmDelete => {
- const bulkActions = [];
- if (canDelete()) {
- bulkActions.push({
- key: 'delete',
- name: (
- <>
- <i className="fa fa-trash" /> {t('Delete')}
- </>
- ),
- onSelect: confirmDelete,
- });
- }
+ const bulkActions: ListViewProps['bulkActions'] = canDelete
+ ? [
+ {
+ key: 'delete',
+ name: t('Delete'),
+ onSelect: confirmDelete,
+ type: 'danger',
+ },
+ ]
+ : [];
+
return (
- <>
- {datasetCurrentlyDeleting && (
- <DeleteModal
- description={t(
- `The dataset ${datasetCurrentlyDeleting.table_name} is linked to
- ${datasetCurrentlyDeleting.chart_count} charts that appear on
- ${datasetCurrentlyDeleting.dashboard_count} dashboards.
- Are you sure you want to continue? Deleting the dataset will break
- those objects.`,
- )}
- onConfirm={() =>
- handleDatasetDelete(datasetCurrentlyDeleting)
- }
- onHide={closeDatasetDeleteModal}
- open
- title={t('Delete Dataset?')}
- />
- )}
- <ListView
- className="dataset-list-view"
- columns={columns}
- data={datasets}
- count={datasetCount}
- pageSize={PAGE_SIZE}
- fetchData={fetchData}
- loading={loading}
- initialSort={initialSort}
- filters={currentFilters}
- bulkActions={bulkActions}
- />
- </>
+ <ListView
+ className="dataset-list-view"
+ columns={columns}
+ data={datasets}
+ count={datasetCount}
+ pageSize={PAGE_SIZE}
+ fetchData={fetchData}
+ loading={loading}
+ initialSort={initialSort}
+ filters={currentFilters}
+ bulkActions={bulkActions}
+ bulkSelectEnabled={bulkSelectEnabled}
+ disableBulkSelect={() => setBulkSelectEnabled(false)}
+ renderBulkSelectCopy={selected => {
+ const { virtualCount, physicalCount } = selected.reduce(
+ (acc, e) => {
+ if (e.original.kind === 'physical') acc.physicalCount += 1;
+ else if (e.original.kind === 'virtual')
+ acc.virtualCount += 1;
+ return acc;
+ },
+ { virtualCount: 0, physicalCount: 0 },
+ );
+
+ if (!selected.length) return t('0 Selected');
+ else if (virtualCount && !physicalCount)
+ return t(
+ '%s Selected (Virtual)',
+ selected.length,
+ virtualCount,
+ );
+ else if (physicalCount && !virtualCount)
+ return t(
+ '%s Selected (Physical)',
+ selected.length,
+ physicalCount,
+ );
Review comment:
can you make this use normal if/else braces? I'm surprised that prettier didn't actually catch this
##########
File path: superset-frontend/spec/helpers/waitForComponentToPaint.js
##########
@@ -0,0 +1,30 @@
+/**
Review comment:
as this is a new file, can it be in typescript?
----------------------------------------------------------------
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] nytai merged pull request #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
nytai merged pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298
----------------------------------------------------------------
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] nytai commented on a change in pull request #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r456116807
##########
File path: superset-frontend/src/components/Button.tsx
##########
@@ -17,41 +17,75 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { kebabCase } from 'lodash';
import {
Button as BootstrapButton,
Tooltip,
OverlayTrigger,
} from 'react-bootstrap';
+import styled from '@superset-ui/style';
-const propTypes = {
- children: PropTypes.node,
- className: PropTypes.string,
- tooltip: PropTypes.node,
- placement: PropTypes.string,
- onClick: PropTypes.func,
- disabled: PropTypes.bool,
- bsSize: PropTypes.string,
- bsStyle: PropTypes.string,
- btnStyles: PropTypes.string,
-};
-const defaultProps = {
- bsSize: 'sm',
- placement: 'top',
-};
+export type OnClickHandler = React.MouseEventHandler<BootstrapButton>;
+
+export interface ButtonProps {
+ className?: string;
+ tooltip?: string;
+ placement?: string;
+ onClick?: OnClickHandler;
+ disabled?: boolean;
+ bsStyle?: string;
+ btnStyles?: string;
+ bsSize?: BootstrapButton.ButtonProps['bsSize'];
+ style?: BootstrapButton.ButtonProps['style'];
+}
const BUTTON_WRAPPER_STYLE = { display: 'inline-block', cursor: 'not-allowed' };
-export default function Button(props) {
- const buttonProps = { ...props };
+const SupersetButton = styled(BootstrapButton)`
+ &.supersetButton {
+ border-radius: ${({ theme }) => theme.gridUnit}px;
+ border: none;
+ color: ${({ theme }) => theme.colors.secondary.light5};
+ font-size: ${({ theme }) => theme.typography.sizes.s};
+ font-weight: ${({ theme }) => theme.typography.weights.bold};
+ min-width: 144px;
+ min-height: 32px;
+ text-transform: uppercase;
+ margin-left: ${({ theme }) => theme.gridUnit * 4}px;
+ &:first-of-type {
+ margin-left: 0;
+ }
+
+ i {
+ padding: 0 ${({ theme }) => theme.gridUnit * 2}px 0 0;
+ }
+
+ &.primary {
+ background-color: ${({ theme }) => theme.colors.primary.base};
+ }
+ &.secondary {
+ color: ${({ theme }) => theme.colors.primary.base};
+ background-color: ${({ theme }) => theme.colors.primary.light4};
+ }
+ &.danger {
+ background-color: ${({ theme }) => theme.colors.error.base};
+ }
+ }
+`;
+
+const Button: React.FunctionComponent<ButtonProps> = props => {
Review comment:
That's a good point, though `React.FunctionComponent<ButtonProps>` adds the react builtin props/children prop.
----------------------------------------------------------------
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 #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#issuecomment-658483062
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=h1) Report
> Merging [#10298](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/7abe3e518298ec896b60e28a8de63e444053e69b&el=desc) will **decrease** coverage by `0.11%`.
> The diff coverage is `88.23%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10298 +/- ##
==========================================
- Coverage 70.58% 70.47% -0.12%
==========================================
Files 600 600
Lines 32093 32185 +92
Branches 3244 3257 +13
==========================================
+ Hits 22654 22682 +28
- Misses 9335 9397 +62
- Partials 104 106 +2
```
| Flag | Coverage Ξ | |
|---|---|---|
| #cypress | `55.09% <71.05%> (-0.38%)` | :arrow_down: |
| #javascript | `59.47% <86.30%> (-0.10%)` | :arrow_down: |
| #python | `69.86% <91.66%> (+0.07%)` | :arrow_up: |
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=tree) | Coverage Ξ | |
|---|---|---|
| [...erset-frontend/src/SqlLab/components/ResultSet.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC50c3g=) | `80.91% <ΓΈ> (ΓΈ)` | |
| [...tend/src/SqlLab/components/ScheduleQueryButton.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NjaGVkdWxlUXVlcnlCdXR0b24uanN4) | `8.62% <ΓΈ> (ΓΈ)` | |
| [superset-frontend/src/SqlLab/constants.ts](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb25zdGFudHMudHM=) | `100.00% <ΓΈ> (ΓΈ)` | |
| [superset-frontend/src/SqlLab/index.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9pbmRleC50c3g=) | `25.00% <ΓΈ> (ΓΈ)` | |
| [superset-frontend/src/chart/Chart.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0LmpzeA==) | `65.30% <0.00%> (-1.37%)` | :arrow_down: |
| [...src/dashboard/components/HeaderActionsDropdown.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0hlYWRlckFjdGlvbnNEcm9wZG93bi5qc3g=) | `79.16% <ΓΈ> (ΓΈ)` | |
| [...d/src/dashboard/components/SliceHeaderControls.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL1NsaWNlSGVhZGVyQ29udHJvbHMuanN4) | `65.95% <ΓΈ> (ΓΈ)` | |
| [.../src/explore/components/ControlPanelsContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9Db250cm9sUGFuZWxzQ29udGFpbmVyLmpzeA==) | `92.30% <ΓΈ> (ΓΈ)` | |
| [...nd/src/explore/components/ExploreActionButtons.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQWN0aW9uQnV0dG9ucy5qc3g=) | `100.00% <ΓΈ> (ΓΈ)` | |
| [...tend/src/explore/components/ExploreChartHeader.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQ2hhcnRIZWFkZXIuanN4) | `82.35% <ΓΈ> (ΓΈ)` | |
| ... and [58 more](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10298?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/10298?src=pr&el=footer). Last update [7abe3e5...b35adea](https://codecov.io/gh/apache/incubator-superset/pull/10298?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] codecov-commenter edited a comment on pull request #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#issuecomment-658483062
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=h1) Report
> Merging [#10298](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/2341e8d5853983f4223e7b6104b4c5bbb488a9b2&el=desc) will **decrease** coverage by `0.05%`.
> The diff coverage is `86.84%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10298 +/- ##
==========================================
- Coverage 70.60% 70.55% -0.06%
==========================================
Files 600 601 +1
Lines 32182 32269 +87
Branches 3257 3275 +18
==========================================
+ Hits 22721 22766 +45
- Misses 9358 9394 +36
- Partials 103 109 +6
```
| Flag | Coverage Ξ | |
|---|---|---|
| #cypress | `55.13% <100.00%> (-0.47%)` | :arrow_down: |
| #javascript | `59.60% <86.84%> (+0.12%)` | :arrow_up: |
| #python | `69.93% <ΓΈ> (+0.03%)` | :arrow_up: |
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/10298?src=pr&el=tree) | Coverage Ξ | |
|---|---|---|
| [...ontend/src/components/ListView/TableCollection.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvVGFibGVDb2xsZWN0aW9uLnRzeA==) | `100.00% <ΓΈ> (+6.89%)` | :arrow_up: |
| [superset-frontend/src/components/ListView/utils.ts](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvdXRpbHMudHM=) | `83.14% <ΓΈ> (ΓΈ)` | |
| [superset-frontend/src/components/Menu/Menu.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTWVudS9NZW51LmpzeA==) | `88.23% <ΓΈ> (ΓΈ)` | |
| [...frontend/src/views/datasetList/AddDatasetModal.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2RhdGFzZXRMaXN0L0FkZERhdGFzZXRNb2RhbC50c3g=) | `58.82% <0.00%> (ΓΈ)` | |
| [...uperset-frontend/src/views/chartList/ChartList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NoYXJ0TGlzdC9DaGFydExpc3QudHN4) | `60.40% <66.66%> (-0.56%)` | :arrow_down: |
| [...frontend/src/views/dashboardList/DashboardList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2Rhc2hib2FyZExpc3QvRGFzaGJvYXJkTGlzdC50c3g=) | `66.00% <75.00%> (-0.44%)` | :arrow_down: |
| [...set-frontend/src/views/datasetList/DatasetList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2RhdGFzZXRMaXN0L0RhdGFzZXRMaXN0LnRzeA==) | `71.34% <83.33%> (+2.85%)` | :arrow_up: |
| [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `68.18% <100.00%> (ΓΈ)` | |
| [superset-frontend/src/components/Button.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQnV0dG9uLnRzeA==) | `93.75% <100.00%> (ΓΈ)` | |
| [...rset-frontend/src/components/ListView/ListView.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvTGlzdFZpZXcudHN4) | `98.30% <100.00%> (+0.34%)` | :arrow_up: |
| ... and [20 more](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10298?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/10298?src=pr&el=footer). Last update [2341e8d...77e4e74](https://codecov.io/gh/apache/incubator-superset/pull/10298?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] lilykuang commented on a change in pull request #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
lilykuang commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r455169393
##########
File path: superset-frontend/src/components/Button.tsx
##########
@@ -17,41 +17,75 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { kebabCase } from 'lodash';
import {
Button as BootstrapButton,
Tooltip,
OverlayTrigger,
} from 'react-bootstrap';
+import styled from '@superset-ui/style';
-const propTypes = {
- children: PropTypes.node,
- className: PropTypes.string,
- tooltip: PropTypes.node,
- placement: PropTypes.string,
- onClick: PropTypes.func,
- disabled: PropTypes.bool,
- bsSize: PropTypes.string,
- bsStyle: PropTypes.string,
- btnStyles: PropTypes.string,
-};
-const defaultProps = {
- bsSize: 'sm',
- placement: 'top',
-};
+export type OnClickHandler = React.MouseEventHandler<BootstrapButton>;
+
+export interface ButtonProps {
+ className?: string;
+ tooltip?: string;
+ placement?: string;
+ onClick?: OnClickHandler;
+ disabled?: boolean;
+ bsStyle?: string;
+ btnStyles?: string;
+ bsSize?: BootstrapButton.ButtonProps['bsSize'];
+ style?: BootstrapButton.ButtonProps['style'];
+}
const BUTTON_WRAPPER_STYLE = { display: 'inline-block', cursor: 'not-allowed' };
-export default function Button(props) {
- const buttonProps = { ...props };
+const SupersetButton = styled(BootstrapButton)`
Review comment:
yes, they are very similar. They should probably be joined together.
----------------------------------------------------------------
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 #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r454534085
##########
File path: superset-frontend/src/components/Menu/SubMenu.tsx
##########
@@ -63,70 +52,63 @@ const StyledHeader = styled.header`
}
`;
-interface SubMenuProps {
- canCreate?: boolean;
- childs?: Array<{ label: string; name: string; url: string }>;
- createButton?: { name: string; url: string | null };
- fetchData?: () => void;
+type MenuChild = {
+ label: string;
name: string;
-}
-
-const SubMenu = ({
- canCreate,
- childs,
- createButton,
- fetchData,
- name,
-}: SubMenuProps) => {
- const [isModalOpen, setIsModalOpen] = useState(false);
- const [selectedMenu, setSelectedMenu] = useState<string | undefined>(
- childs?.[0]?.label,
- );
-
- const onOpen = () => {
- setIsModalOpen(true);
- };
+ url: string;
+};
- const onClose = () => {
- setIsModalOpen(false);
+export interface SubMenuProps {
+ primaryButton?: {
+ name: React.ReactNode;
+ onClick: OnClickHandler;
};
-
- const handleClick = (item: string) => () => {
- setSelectedMenu(item);
+ secondaryButton?: {
+ name: React.ReactNode;
+ onClick: OnClickHandler;
};
+ name: string;
+ children?: MenuChild[];
+ activeChild?: MenuChild['name'];
+}
+const SubMenu: React.FunctionComponent<SubMenuProps> = props => {
return (
<StyledHeader>
<Navbar inverse fluid role="navigation">
<Navbar.Header>
- <Navbar.Brand>{name}</Navbar.Brand>
+ <Navbar.Brand>{props.name}</Navbar.Brand>
</Navbar.Header>
- <DatasetModal
- fetchData={fetchData}
- onHide={onClose}
- show={isModalOpen}
- />
<Nav>
- {childs &&
- childs.map(child => (
+ {props.children &&
Review comment:
π that name always rubbed me the wrong 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 #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#issuecomment-658483062
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=h1) Report
> Merging [#10298](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/2341e8d5853983f4223e7b6104b4c5bbb488a9b2&el=desc) will **decrease** coverage by `4.96%`.
> The diff coverage is `86.84%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10298?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10298 +/- ##
==========================================
- Coverage 70.60% 65.63% -4.97%
==========================================
Files 600 601 +1
Lines 32182 32268 +86
Branches 3257 3275 +18
==========================================
- Hits 22721 21178 -1543
- Misses 9358 10906 +1548
- Partials 103 184 +81
```
| Flag | Coverage Ξ | |
|---|---|---|
| #cypress | `?` | |
| #javascript | `59.60% <86.84%> (+0.12%)` | :arrow_up: |
| #python | `69.86% <ΓΈ> (-0.04%)` | :arrow_down: |
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/10298?src=pr&el=tree) | Coverage Ξ | |
|---|---|---|
| [...ontend/src/components/ListView/TableCollection.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvVGFibGVDb2xsZWN0aW9uLnRzeA==) | `100.00% <ΓΈ> (+6.89%)` | :arrow_up: |
| [superset-frontend/src/components/ListView/utils.ts](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvdXRpbHMudHM=) | `83.14% <ΓΈ> (ΓΈ)` | |
| [superset-frontend/src/components/Menu/Menu.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTWVudS9NZW51LmpzeA==) | `88.23% <ΓΈ> (ΓΈ)` | |
| [...frontend/src/views/datasetList/AddDatasetModal.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2RhdGFzZXRMaXN0L0FkZERhdGFzZXRNb2RhbC50c3g=) | `58.82% <0.00%> (ΓΈ)` | |
| [...uperset-frontend/src/views/chartList/ChartList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NoYXJ0TGlzdC9DaGFydExpc3QudHN4) | `60.40% <66.66%> (-0.56%)` | :arrow_down: |
| [...frontend/src/views/dashboardList/DashboardList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2Rhc2hib2FyZExpc3QvRGFzaGJvYXJkTGlzdC50c3g=) | `66.00% <75.00%> (-0.44%)` | :arrow_down: |
| [...set-frontend/src/views/datasetList/DatasetList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2RhdGFzZXRMaXN0L0RhdGFzZXRMaXN0LnRzeA==) | `71.34% <83.33%> (+2.85%)` | :arrow_up: |
| [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `68.18% <100.00%> (ΓΈ)` | |
| [superset-frontend/src/components/Button.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQnV0dG9uLnRzeA==) | `93.75% <100.00%> (ΓΈ)` | |
| [...rset-frontend/src/components/ListView/ListView.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvTGlzdFZpZXcudHN4) | `98.30% <100.00%> (+0.34%)` | :arrow_up: |
| ... and [169 more](https://codecov.io/gh/apache/incubator-superset/pull/10298/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10298?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/10298?src=pr&el=footer). Last update [2341e8d...77e4e74](https://codecov.io/gh/apache/incubator-superset/pull/10298?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 #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r454529270
##########
File path: superset-frontend/src/components/ListView/ListViewStyles.less
##########
@@ -104,8 +104,16 @@
}
.table-row {
+ .actions {
+ visibility: hidden;
Review comment:
let's have a little fun here ;)
```suggestion
opacity: 0;
transition: opacity ease-in @timing-normal;
```
----------------------------------------------------------------
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 #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
rusackas commented on pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#issuecomment-679293391
Impacts #8976
----------------------------------------------------------------
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 #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r454540720
##########
File path: superset-frontend/src/views/dashboardList/DashboardList.tsx
##########
@@ -45,23 +45,24 @@ interface Props {
}
interface State {
- dashboards: any[];
+ bulkSelectEnabled: boolean;
dashboardCount: number;
- loading: boolean;
+ dashboards: any[];
Review comment:
Not sure if it's worth the effort, but if instead of `dashboard____ ` keys, these could be made more generic, e.g. `itemCount`, and then `ListView` could export a shared/reusable interface?
----------------------------------------------------------------
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] nytai commented on a change in pull request #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r454558309
##########
File path: superset-frontend/src/components/ListView/ListView.tsx
##########
@@ -155,6 +205,37 @@ const ListView: FunctionComponent<Props> = ({
)}
</div>
<div className="body">
+ {bulkSelectEnabled && (
+ <BulkSelectWrapper bsStyle="info" onDismiss={disableBulkSelect}>
+ <div className="selectedCopy">
+ {renderBulkSelectCopy(selectedFlatRows)}
+ </div>
+ {Boolean(selectedFlatRows.length) && (
+ <>
+ <span
+ role="button"
+ tabIndex={0}
+ className="deselect-all"
+ onClick={() => toggleAllRowsSelected(false)}
+ >
+ {t('Deselect All')}
+ </span>
+ <div className="divider" />
+ {bulkActions.map(action => (
+ <Button
Review comment:
re: the SupersetButton comment, do you think it makes more sense to just make a new SupersetButton component and make the split between the new and the old explicit, or does having one component serving both make sense?
----------------------------------------------------------------
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 #10298: feat(listviews): SIP-34 Bulk Select
Posted by GitBox <gi...@apache.org>.
rusackas commented on pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#issuecomment-658334150
Added some questions/comments/etc, but this is looking great!
----------------------------------------------------------------
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