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/06/18 02:44:34 UTC
[GitHub] [incubator-superset] nytai opened a new pull request #10094: style: listviews to match SIP-34
nytai opened a new pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This changes styles and pagination to match SIP-34 and improves loading of the listview. The listview doesn't render until data is fetched and fetching new data uses a skeleton shimmer loading effect.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
<img width="3008" alt="Screen Shot 2020-06-17 at 5 43 59 PM" src="https://user-images.githubusercontent.com/10255196/84971906-9ce38a80-b0d2-11ea-9a46-6e4e6fb99c42.png">
<img width="1672" alt="Screen Shot 2020-06-17 at 7 34 45 PM" src="https://user-images.githubusercontent.com/10255196/84971915-a10fa800-b0d2-11ea-83b6-3c092bddfb5a.png">
Dashboards and Charts
<img width="3008" alt="Screen Shot 2020-06-17 at 5 43 33 PM" src="https://user-images.githubusercontent.com/10255196/84971864-863d3380-b0d2-11ea-9b97-eda7be1c5ef5.png">
<img width="3006" alt="Screen Shot 2020-06-17 at 5 43 44 PM" src="https://user-images.githubusercontent.com/10255196/84971879-8c331480-b0d2-11ea-89b2-010d066283d2.png">
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r444037556
##########
File path: superset-frontend/src/types/react-table-config.d.ts
##########
@@ -64,8 +64,10 @@ import {
UseSortByOptions,
UseSortByState,
} from 'react-table';
+import { ColumnSizer } from 'react-virtualized';
declare module 'react-table' {
+ type columnSize = 'xs' | 'sm' | 'md' | 'lg' | 'xl';
Review comment:
This probably should be `ColumnSize`, especially since that's how it's referenced on line 134
##########
File path: superset-frontend/src/components/AvatarIcon.tsx
##########
@@ -42,18 +43,26 @@ export default function AvatarIcon({
lastName,
userName,
iconSize,
+ textSize,
}: Props) {
const uniqueKey = `${tableName}-${userName}`;
const fullName = `${firstName} ${lastName}`;
return (
- <ConfigProvider colors={colorList && colorList.colors}>
- <OverlayTrigger
- placement="right"
- overlay={<Tooltip id={`${uniqueKey}-tooltip`}>{fullName}</Tooltip>}
- >
- <StyledAvatar key={uniqueKey} name={fullName} size={iconSize} round />
- </OverlayTrigger>
- </ConfigProvider>
+ <TooltipWrapper
+ placement="bottom"
+ label={`${uniqueKey}-tooltip`}
+ tooltip={fullName}
+ >
+ <ConfigProvider colors={colorList && colorList.colors}>
+ <StyledAvatar
+ key={uniqueKey}
+ name={fullName}
+ size={String(iconSize)}
+ textSizeRatio={Number(iconSize) / textSize}
Review comment:
iconSize is typed as a number, so i don't think you need to wrap it in `Number()` here
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r444511579
##########
File path: superset-frontend/src/components/AvatarIcon.tsx
##########
@@ -19,15 +19,16 @@
import React from 'react';
import styled from '@superset-ui/style';
import { getCategoricalSchemeRegistry } from '@superset-ui/color';
-import { Tooltip, OverlayTrigger } from 'react-bootstrap';
import Avatar, { ConfigProvider } from 'react-avatar';
+import TooltipWrapper from 'src/components/TooltipWrapper';
Review comment:
The reason it's TooltipWrapper is because it allows for any `tooltip` to be rendered with the correct triggers. `tooltip` is a prop of tooltipWrapper that defaults to using the bootstrap tooltip. You could use another tooltip component and use TooltipWrapper to ensure it renders when appropriate.
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r444472545
##########
File path: superset-frontend/src/components/AvatarIcon.tsx
##########
@@ -19,15 +19,16 @@
import React from 'react';
import styled from '@superset-ui/style';
import { getCategoricalSchemeRegistry } from '@superset-ui/color';
-import { Tooltip, OverlayTrigger } from 'react-bootstrap';
import Avatar, { ConfigProvider } from 'react-avatar';
+import TooltipWrapper from 'src/components/TooltipWrapper';
Review comment:
... and if this was due to namespacing issues, I would just rename the internal import, e.g. `import { Tooltip as RB_Tooltip } from 'react-bootstrap';`
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r442634389
##########
File path: superset-frontend/src/components/ListView/ListView.tsx
##########
@@ -116,124 +118,117 @@ const ListView: FunctionComponent<Props> = ({
}
});
}
-
+ if (loading && !data.length) {
+ return <Loading />;
+ }
return (
- <div className={`superset-list-view ${className}`}>
- <div className="header">
- {!useNewUIFilters && (
- <>
- {title && filterable && (
- <>
- <Row>
- <Col md={11}>
- <h2>{t(title)}</h2>
- </Col>
- {filterable && (
- <Col md={1}>
- <FilterMenu
- filters={filters}
- internalFilters={internalFilters}
- setInternalFilters={setInternalFilters}
- />
- </Col>
- )}
- </Row>
- <hr />
- <FilterInputs
- internalFilters={internalFilters}
- filters={filters}
- updateInternalFilter={updateInternalFilter}
- removeFilterAndApply={removeFilterAndApply}
- filtersApplied={filtersApplied}
- applyFilters={applyFilters}
- />
- </>
- )}
- </>
- )}
- {useNewUIFilters && (
- <>
- <Row>
- <Col md={10}>
- <h2>{t(title)}</h2>
- </Col>
- </Row>
- <hr />
+ <div className="superset-list-view-container">
+ <div className={`superset-list-view ${className}`}>
+ <div className="header">
+ {!useNewUIFilters && (
+ <>
Review comment:
do you need this fragment here? in fact, it seems like some extra nesting with !useNewUIFilers, title, and filterable can be joined into a single statement
##########
File path: superset-frontend/src/components/ListView/ListViewStyles.less
##########
@@ -19,92 +19,127 @@
@import '~stylesheets/less/variables.less';
-.superset-list-view {
- .filter-dropdown {
- margin-top: 20px;
- }
+.superset-list-view-container {
+ text-align: center;
+
+ .superset-list-view {
+ text-align: left;
+ background-color: white;
+ border-radius: 4px 0;
+ overflow: scroll;
+ margin: 0 16px;
+ padding-bottom: 48px;
+
+ .filter-dropdown {
+ margin-top: 20px;
+ }
- .filter-column {
- height: 30px;
- padding: 5px;
- font-size: 16px;
- }
+ .filter-column {
+ height: 30px;
+ padding: 5px;
+ font-size: 16px;
+ }
- .filter-close {
- height: 30px;
- padding: 5px;
+ .filter-close {
+ height: 30px;
+ padding: 5px;
- i {
- font-size: 20px;
+ i {
+ font-size: 20px;
+ }
}
- }
- .table-row-loader {
- animation: shimmer 2s infinite;
- background: linear-gradient(
- to right,
- #f6f7f8 0%,
- #edeef1 20%,
- #f6f7f8 40%,
- #f6f7f8 100%
- );
- background-size: 1000px 100%;
-
- span {
- visibility: hidden;
+ .table-cell-loader {
+ position: relative;
+
+ .loading-bar {
+ background-color: #e7e7e7;
Review comment:
do we have a variable for this one?
##########
File path: superset-frontend/src/views/chartList/ChartList.tsx
##########
@@ -517,58 +519,55 @@ class ChartList extends React.PureComponent<Props, State> {
sliceCurrentlyEditing,
} = this.state;
return (
- <div className="container welcome">
- <Panel>
- <Panel.Body>
- {sliceCurrentlyEditing && (
- <PropertiesModal
- show
- onHide={this.closeChartEditModal}
- onSave={this.handleChartUpdated}
- slice={sliceCurrentlyEditing}
+ <>
+ <SubMenu name="Charts" />
+ {sliceCurrentlyEditing && (
+ <PropertiesModal
+ show
+ onHide={this.closeChartEditModal}
+ onSave={this.handleChartUpdated}
+ slice={sliceCurrentlyEditing}
+ />
+ )}
+ <ConfirmStatusChange
+ title={t('Please confirm')}
+ description={t(
+ 'Are you sure you want to delete the selected charts?',
+ )}
+ onConfirm={this.handleBulkChartDelete}
+ >
+ {confirmDelete => {
+ const bulkActions = [];
+ if (this.canDelete) {
+ bulkActions.push({
+ key: 'delete',
+ name: (
+ <>
+ <i className="fa fa-trash" /> Delete
+ </>
+ ),
+ onSelect: confirmDelete,
+ });
+ }
+ return (
+ <ListView
+ className="chart-list-view"
+ title={'Charts'}
Review comment:
nit: `t` string here please!
##########
File path: superset-frontend/src/types/react-table-config.d.ts
##########
@@ -118,13 +118,19 @@ declare module 'react-table' {
hidden?: boolean;
sortable?: boolean;
cellProps?: any;
+ size?: 'xs' | 'sm' | 'md' | 'lg' | 'xl';
Review comment:
Let's define these strings as it's own `Size` type then reuse it in both places
##########
File path: superset-frontend/src/views/dashboardList/DashboardList.tsx
##########
@@ -508,71 +510,68 @@ class DashboardList extends React.PureComponent<Props, State> {
dashboardToEdit,
} = this.state;
return (
- <div className="container welcome">
- <Panel>
- <Panel.Body>
- <ConfirmStatusChange
- title={t('Please confirm')}
- description={t(
- 'Are you sure you want to delete the selected dashboards?',
- )}
- onConfirm={this.handleBulkDashboardDelete}
- >
- {confirmDelete => {
- const bulkActions = [];
- if (this.canDelete) {
- bulkActions.push({
- key: 'delete',
- name: (
- <>
- <i className="fa fa-trash" /> Delete
- </>
- ),
- onSelect: confirmDelete,
- });
- }
- if (this.canExport) {
- bulkActions.push({
- key: 'export',
- name: (
- <>
- <i className="fa fa-database" /> Export
- </>
- ),
- onSelect: this.handleBulkDashboardExport,
- });
- }
- return (
+ <>
Review comment:
it seems like a lot of code is the same as in the above file, is there any way we can pull it out into its own component and reuse it?
##########
File path: superset-frontend/src/views/datasetList/DatasetList.tsx
##########
@@ -487,49 +524,43 @@ class DatasetList extends React.PureComponent<Props, State> {
return (
<>
<SubMenu {...this.menu} canCreate={this.canCreate} />
- <div className="container welcome">
- <Panel>
- <Panel.Body>
- <ConfirmStatusChange
- title={t('Please confirm')}
- description={t(
- 'Are you sure you want to delete the selected datasets?',
- )}
- onConfirm={this.handleBulkDatasetDelete}
- >
- {confirmDelete => {
- const bulkActions = [];
- if (this.canDelete) {
- bulkActions.push({
- key: 'delete',
- name: (
- <>
- <i className="fa fa-trash" /> Delete
- </>
- ),
- onSelect: confirmDelete,
- });
- }
- return (
- <ListView
- className="dataset-list-view"
- title={'Datasets'}
- columns={this.columns}
- data={datasets}
- count={datasetCount}
- pageSize={PAGE_SIZE}
- fetchData={this.fetchData}
- loading={loading}
- initialSort={this.initialSort}
- filters={filters}
- bulkActions={bulkActions}
- />
- );
- }}
- </ConfirmStatusChange>
- </Panel.Body>
- </Panel>
- </div>
+ <ConfirmStatusChange
+ title={t('Please confirm')}
+ description={t(
+ 'Are you sure you want to delete the selected datasets?',
+ )}
+ onConfirm={this.handleBulkDatasetDelete}
+ >
+ {confirmDelete => {
+ const bulkActions = [];
+ if (this.canDelete) {
+ bulkActions.push({
+ key: 'delete',
+ name: (
+ <>
+ <i className="fa fa-trash" /> Delete
Review comment:
nit: `t` string here please!
##########
File path: superset-frontend/src/components/ListView/ListView.tsx
##########
@@ -116,124 +118,117 @@ const ListView: FunctionComponent<Props> = ({
}
});
}
-
+ if (loading && !data.length) {
+ return <Loading />;
+ }
return (
- <div className={`superset-list-view ${className}`}>
- <div className="header">
- {!useNewUIFilters && (
- <>
- {title && filterable && (
- <>
- <Row>
- <Col md={11}>
- <h2>{t(title)}</h2>
- </Col>
- {filterable && (
- <Col md={1}>
- <FilterMenu
- filters={filters}
- internalFilters={internalFilters}
- setInternalFilters={setInternalFilters}
- />
- </Col>
- )}
- </Row>
- <hr />
- <FilterInputs
- internalFilters={internalFilters}
- filters={filters}
- updateInternalFilter={updateInternalFilter}
- removeFilterAndApply={removeFilterAndApply}
- filtersApplied={filtersApplied}
- applyFilters={applyFilters}
- />
- </>
- )}
- </>
- )}
- {useNewUIFilters && (
- <>
- <Row>
- <Col md={10}>
- <h2>{t(title)}</h2>
- </Col>
- </Row>
- <hr />
+ <div className="superset-list-view-container">
+ <div className={`superset-list-view ${className}`}>
+ <div className="header">
+ {!useNewUIFilters && (
+ <>
+ {title && filterable && (
+ <>
+ <Row>
+ <Col md={10} />
+ {filterable && (
+ <Col md={2}>
+ <FilterMenu
+ filters={filters}
+ internalFilters={internalFilters}
+ setInternalFilters={setInternalFilters}
+ />
+ </Col>
+ )}
+ </Row>
+ <hr />
+ <FilterInputs
+ internalFilters={internalFilters}
+ filters={filters}
+ updateInternalFilter={updateInternalFilter}
+ removeFilterAndApply={removeFilterAndApply}
+ filtersApplied={filtersApplied}
+ applyFilters={applyFilters}
+ />
+ </>
+ )}
+ </>
+ )}
+ {useNewUIFilters && (
<FilterControls
filters={filters}
internalFilters={internalFilters}
updateFilterValue={applyFilterValue}
/>
- </>
- )}
- </div>
- <div className="body">
- <TableCollection
- getTableProps={getTableProps}
- getTableBodyProps={getTableBodyProps}
- prepareRow={prepareRow}
- headerGroups={headerGroups}
- rows={rows}
- loading={loading}
- />
- </div>
- <div className="footer">
- <Row>
- <Col md={2}>
- <div className="form-actions-container">
- <div className="btn-group">
- {bulkActions.length > 0 && (
- <DropdownButton
- id="bulk-actions"
- bsSize="small"
- bsStyle="default"
- noCaret
- title={
- <>
- {t('Actions')} <span className="caret" />
- </>
- }
- >
- {bulkActions.map(action => (
- // @ts-ignore
- <MenuItem
- key={action.key}
- eventKey={selectedFlatRows}
+ )}
+ </div>
+ <div className="body">
+ <TableCollection
+ getTableProps={getTableProps}
+ getTableBodyProps={getTableBodyProps}
+ prepareRow={prepareRow}
+ headerGroups={headerGroups}
+ rows={rows}
+ loading={loading}
+ />
+ </div>
+ <div className="footer">
+ <Row>
+ <Col>
+ <div className="form-actions-container">
+ <div className="btn-group">
+ {bulkActions.length > 0 && (
+ <DropdownButton
+ id="bulk-actions"
+ bsSize="small"
+ bsStyle="default"
+ noCaret
+ title={
+ <>
+ {t('Actions')} <span className="caret" />
+ </>
+ }
+ >
+ {bulkActions.map(action => (
// @ts-ignore
- onSelect={(selectedRows: typeof selectedFlatRows) => {
- action.onSelect(
- selectedRows.map((r: any) => r.original),
- );
- }}
- >
- {action.name}
- </MenuItem>
- ))}
- </DropdownButton>
- )}
+ <MenuItem
+ key={action.key}
+ eventKey={selectedFlatRows}
+ // @ts-ignore
+ onSelect={(selectedRows: typeof selectedFlatRows) => {
+ action.onSelect(
+ selectedRows.map((r: any) => r.original),
+ );
+ }}
+ >
+ {action.name}
+ </MenuItem>
+ ))}
+ </DropdownButton>
+ )}
+ </div>
</div>
- </div>
- </Col>
- <Col md={8} className="text-center">
- <Pagination
- totalPages={pageCount || 0}
- currentPage={pageCount ? pageIndex + 1 : 0}
- onChange={(p: number) => gotoPage(p - 1)}
- hideFirstAndLastPageLinks
- />
- </Col>
- <Col md={2}>
- <span className="pull-right">
- {t('showing')}{' '}
- <strong>
- {pageSize * pageIndex + (rows.length && 1)}-
- {pageSize * pageIndex + rows.length}
- </strong>{' '}
- {t('of')} <strong>{count}</strong>
- </span>
- </Col>
- </Row>
+ </Col>
+
+ <Col>
+ <span className="row-count-container">
+ {t('showing')}{' '}
+ <strong>
+ {pageSize * pageIndex + (rows.length && 1)}-
+ {pageSize * pageIndex + rows.length}
+ </strong>{' '}
+ {t('of')} <strong>{count}</strong>
Review comment:
these translation strings won't work at all. I think using string replacement is required instead, details are here: https://github.com/apache-superset/superset-ui/tree/master/packages/superset-ui-translation
although, now that i look closer, it seems like these are just indentation changes. so i guess it's up to you if you want to fix them or not. not sure how they snuck past in the first place :/ if it's not in scope to fix them here, let me know and i can clean up afterwards
##########
File path: superset-frontend/src/components/ListView/TableCollection.tsx
##########
@@ -29,6 +30,18 @@ interface Props {
rows: TableInstance['rows'];
loading: boolean;
}
+
+const TableCell = styled.td`
+ width: ${(props: { size?: Column['size'] }) => {
+ if (props.size === 'xs') return '25px';
+ if (props.size === 'sm') return '50px';
+ if (props.size === 'md') return '75px';
+ if (props.size === 'lg') return '100px';
+ if (props.size === 'xl') return '150px';
+ if (props.size === 'xxl') return '200px';
+ return '';
+ }};
Review comment:
this could be simpler if you have each if only return a number, then you append `px` on at the end. Note the final return would need to return 0 instead of empty string then.
on a side note, do you think a switch statement is better here? it would remove even more code duplication
##########
File path: superset-frontend/src/components/ListView/ListViewStyles.less
##########
@@ -19,92 +19,127 @@
@import '~stylesheets/less/variables.less';
-.superset-list-view {
- .filter-dropdown {
- margin-top: 20px;
- }
+.superset-list-view-container {
+ text-align: center;
+
+ .superset-list-view {
+ text-align: left;
+ background-color: white;
+ border-radius: 4px 0;
+ overflow: scroll;
+ margin: 0 16px;
+ padding-bottom: 48px;
+
+ .filter-dropdown {
+ margin-top: 20px;
+ }
- .filter-column {
- height: 30px;
- padding: 5px;
- font-size: 16px;
- }
+ .filter-column {
+ height: 30px;
+ padding: 5px;
+ font-size: 16px;
+ }
- .filter-close {
- height: 30px;
- padding: 5px;
+ .filter-close {
+ height: 30px;
+ padding: 5px;
- i {
- font-size: 20px;
+ i {
+ font-size: 20px;
+ }
}
- }
- .table-row-loader {
- animation: shimmer 2s infinite;
- background: linear-gradient(
- to right,
- #f6f7f8 0%,
- #edeef1 20%,
- #f6f7f8 40%,
- #f6f7f8 100%
- );
- background-size: 1000px 100%;
-
- span {
- visibility: hidden;
+ .table-cell-loader {
+ position: relative;
+
+ .loading-bar {
+ background-color: #e7e7e7;
+ border-radius: 7px;
+
+ span {
+ visibility: hidden;
+ }
+ }
+
+ &:after {
+ position: absolute;
+ transform: translateY(-50%);
+ top: 50%;
+ left: 0;
+ content: '';
+ display: block;
+ width: 100%;
+ height: 48px;
+ background-image: linear-gradient(
+ 100deg,
+ rgba(255, 255, 255, 0),
+ rgba(255, 255, 255, 0.5) 60%,
+ rgba(255, 255, 255, 0) 80%
Review comment:
feels like we might have variables for these colors already, although i'm not sure. Maybe we should add them if not?
##########
File path: superset-frontend/src/components/Pagination.tsx
##########
@@ -0,0 +1,132 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { PureComponent } from 'react';
+import cx from 'classnames';
+import styled from '@superset-ui/style';
+
+interface PaginationButton {
+ disabled?: boolean;
+ onClick: React.EventHandler<React.SyntheticEvent<HTMLElement>>;
+}
+
+interface PaginationItemButton extends PaginationButton {
+ active: boolean;
+ children: React.ReactNode;
+}
+
+function Prev({ disabled, onClick }: PaginationButton) {
+ return (
+ <li className={cx({ disabled })}>
+ <span role="button" tabIndex={disabled ? -1 : 0} onClick={onClick}>
+ «
Review comment:
is using unicode a good idea for this, or should we add svg icons while we're in here making stuff closer to SIP-34?
##########
File path: superset-frontend/src/views/dashboardList/DashboardList.tsx
##########
@@ -508,71 +510,68 @@ class DashboardList extends React.PureComponent<Props, State> {
dashboardToEdit,
} = this.state;
return (
- <div className="container welcome">
- <Panel>
- <Panel.Body>
- <ConfirmStatusChange
- title={t('Please confirm')}
- description={t(
- 'Are you sure you want to delete the selected dashboards?',
- )}
- onConfirm={this.handleBulkDashboardDelete}
- >
- {confirmDelete => {
- const bulkActions = [];
- if (this.canDelete) {
- bulkActions.push({
- key: 'delete',
- name: (
- <>
- <i className="fa fa-trash" /> Delete
- </>
- ),
- onSelect: confirmDelete,
- });
- }
- if (this.canExport) {
- bulkActions.push({
- key: 'export',
- name: (
- <>
- <i className="fa fa-database" /> Export
- </>
- ),
- onSelect: this.handleBulkDashboardExport,
- });
- }
- return (
+ <>
+ <SubMenu name="Dashboards" />
+ <ConfirmStatusChange
+ title={t('Please confirm')}
+ description={t(
+ 'Are you sure you want to delete the selected dashboards?',
+ )}
+ onConfirm={this.handleBulkDashboardDelete}
+ >
+ {confirmDelete => {
+ const bulkActions = [];
+ if (this.canDelete) {
+ bulkActions.push({
+ key: 'delete',
+ name: (
+ <>
+ <i className="fa fa-trash" /> Delete
+ </>
+ ),
+ onSelect: confirmDelete,
+ });
+ }
+ if (this.canExport) {
+ bulkActions.push({
+ key: 'export',
+ name: (
<>
- {dashboardToEdit && (
- <PropertiesModal
- show
- dashboardId={dashboardToEdit.id}
- onHide={() => this.setState({ dashboardToEdit: null })}
- onDashboardSave={this.handleDashboardEdit}
- />
- )}
- <ListView
- className="dashboard-list-view"
- title={'Dashboards'}
- columns={this.columns}
- data={dashboards}
- count={dashboardCount}
- pageSize={PAGE_SIZE}
- fetchData={this.fetchData}
- loading={loading}
- initialSort={this.initialSort}
- filters={filters}
- bulkActions={bulkActions}
- useNewUIFilters={this.isNewUIEnabled}
- />
+ <i className="fa fa-database" /> Export
</>
- );
- }}
- </ConfirmStatusChange>
- </Panel.Body>
- </Panel>
- </div>
+ ),
+ onSelect: this.handleBulkDashboardExport,
+ });
+ }
+ return (
+ <>
+ {dashboardToEdit && (
+ <PropertiesModal
+ show
+ dashboardId={dashboardToEdit.id}
+ onHide={() => this.setState({ dashboardToEdit: null })}
+ onDashboardSave={this.handleDashboardEdit}
+ />
+ )}
+ <ListView
+ className="dashboard-list-view"
+ title={'Dashboards'}
Review comment:
nit, `t` string here please!
##########
File path: superset-frontend/src/components/Pagination.tsx
##########
@@ -0,0 +1,132 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { PureComponent } from 'react';
+import cx from 'classnames';
+import styled from '@superset-ui/style';
+
+interface PaginationButton {
+ disabled?: boolean;
+ onClick: React.EventHandler<React.SyntheticEvent<HTMLElement>>;
+}
+
+interface PaginationItemButton extends PaginationButton {
+ active: boolean;
+ children: React.ReactNode;
+}
+
+function Prev({ disabled, onClick }: PaginationButton) {
+ return (
+ <li className={cx({ disabled })}>
+ <span role="button" tabIndex={disabled ? -1 : 0} onClick={onClick}>
+ «
+ </span>
+ </li>
+ );
+}
+
+function Next({ disabled, onClick }: PaginationButton) {
+ return (
+ <li className={cx({ disabled })}>
+ <span role="button" tabIndex={disabled ? -1 : 0} onClick={onClick}>
+ »
+ </span>
+ </li>
+ );
+}
+
+function Item({ active, children, onClick }: PaginationItemButton) {
+ return (
+ <li className={cx({ active })}>
+ <span role="button" tabIndex={active ? -1 : 0} onClick={onClick}>
+ {children}
+ </span>
+ </li>
+ );
+}
+
+function Ellipsis({ disabled, onClick }: PaginationButton) {
+ return (
+ <li className={cx({ disabled })}>
+ <span role="button" tabIndex={disabled ? -1 : 0} onClick={onClick}>
+ ...
Review comment:
should this be `...` or `…` (3 periods vs. an ellipsis)?
##########
File path: superset-frontend/src/views/datasetList/DatasetList.tsx
##########
@@ -487,49 +524,43 @@ class DatasetList extends React.PureComponent<Props, State> {
return (
<>
<SubMenu {...this.menu} canCreate={this.canCreate} />
- <div className="container welcome">
- <Panel>
- <Panel.Body>
- <ConfirmStatusChange
- title={t('Please confirm')}
- description={t(
- 'Are you sure you want to delete the selected datasets?',
- )}
- onConfirm={this.handleBulkDatasetDelete}
- >
- {confirmDelete => {
- const bulkActions = [];
- if (this.canDelete) {
- bulkActions.push({
- key: 'delete',
- name: (
- <>
- <i className="fa fa-trash" /> Delete
- </>
- ),
- onSelect: confirmDelete,
- });
- }
- return (
- <ListView
- className="dataset-list-view"
- title={'Datasets'}
- columns={this.columns}
- data={datasets}
- count={datasetCount}
- pageSize={PAGE_SIZE}
- fetchData={this.fetchData}
- loading={loading}
- initialSort={this.initialSort}
- filters={filters}
- bulkActions={bulkActions}
- />
- );
- }}
- </ConfirmStatusChange>
- </Panel.Body>
- </Panel>
- </div>
+ <ConfirmStatusChange
+ title={t('Please confirm')}
+ description={t(
+ 'Are you sure you want to delete the selected datasets?',
+ )}
+ onConfirm={this.handleBulkDatasetDelete}
+ >
+ {confirmDelete => {
+ const bulkActions = [];
+ if (this.canDelete) {
+ bulkActions.push({
+ key: 'delete',
+ name: (
+ <>
+ <i className="fa fa-trash" /> Delete
+ </>
+ ),
+ onSelect: confirmDelete,
+ });
+ }
+ return (
+ <ListView
+ className="dataset-list-view"
+ title={'Datasets'}
Review comment:
nit: `t` string here please!
##########
File path: superset-frontend/src/views/dashboardList/DashboardList.tsx
##########
@@ -508,71 +510,68 @@ class DashboardList extends React.PureComponent<Props, State> {
dashboardToEdit,
} = this.state;
return (
- <div className="container welcome">
- <Panel>
- <Panel.Body>
- <ConfirmStatusChange
- title={t('Please confirm')}
- description={t(
- 'Are you sure you want to delete the selected dashboards?',
- )}
- onConfirm={this.handleBulkDashboardDelete}
- >
- {confirmDelete => {
- const bulkActions = [];
- if (this.canDelete) {
- bulkActions.push({
- key: 'delete',
- name: (
- <>
- <i className="fa fa-trash" /> Delete
- </>
- ),
- onSelect: confirmDelete,
- });
- }
- if (this.canExport) {
- bulkActions.push({
- key: 'export',
- name: (
- <>
- <i className="fa fa-database" /> Export
- </>
- ),
- onSelect: this.handleBulkDashboardExport,
- });
- }
- return (
+ <>
+ <SubMenu name="Dashboards" />
+ <ConfirmStatusChange
+ title={t('Please confirm')}
+ description={t(
+ 'Are you sure you want to delete the selected dashboards?',
+ )}
+ onConfirm={this.handleBulkDashboardDelete}
+ >
+ {confirmDelete => {
+ const bulkActions = [];
+ if (this.canDelete) {
+ bulkActions.push({
+ key: 'delete',
+ name: (
+ <>
+ <i className="fa fa-trash" /> Delete
Review comment:
nit: `t` string here please!
##########
File path: superset-frontend/src/views/chartList/ChartList.tsx
##########
@@ -517,58 +519,55 @@ class ChartList extends React.PureComponent<Props, State> {
sliceCurrentlyEditing,
} = this.state;
return (
- <div className="container welcome">
- <Panel>
- <Panel.Body>
- {sliceCurrentlyEditing && (
- <PropertiesModal
- show
- onHide={this.closeChartEditModal}
- onSave={this.handleChartUpdated}
- slice={sliceCurrentlyEditing}
+ <>
+ <SubMenu name="Charts" />
Review comment:
nit: `t` string here please!
##########
File path: superset-frontend/src/views/chartList/ChartList.tsx
##########
@@ -517,58 +519,55 @@ class ChartList extends React.PureComponent<Props, State> {
sliceCurrentlyEditing,
} = this.state;
return (
- <div className="container welcome">
- <Panel>
- <Panel.Body>
- {sliceCurrentlyEditing && (
- <PropertiesModal
- show
- onHide={this.closeChartEditModal}
- onSave={this.handleChartUpdated}
- slice={sliceCurrentlyEditing}
+ <>
+ <SubMenu name="Charts" />
+ {sliceCurrentlyEditing && (
+ <PropertiesModal
+ show
+ onHide={this.closeChartEditModal}
+ onSave={this.handleChartUpdated}
+ slice={sliceCurrentlyEditing}
+ />
+ )}
+ <ConfirmStatusChange
+ title={t('Please confirm')}
+ description={t(
+ 'Are you sure you want to delete the selected charts?',
+ )}
+ onConfirm={this.handleBulkChartDelete}
+ >
+ {confirmDelete => {
+ const bulkActions = [];
+ if (this.canDelete) {
+ bulkActions.push({
+ key: 'delete',
+ name: (
+ <>
+ <i className="fa fa-trash" /> Delete
Review comment:
nit: `t` string here please!
##########
File path: superset-frontend/src/views/dashboardList/DashboardList.tsx
##########
@@ -508,71 +510,68 @@ class DashboardList extends React.PureComponent<Props, State> {
dashboardToEdit,
} = this.state;
return (
- <div className="container welcome">
- <Panel>
- <Panel.Body>
- <ConfirmStatusChange
- title={t('Please confirm')}
- description={t(
- 'Are you sure you want to delete the selected dashboards?',
- )}
- onConfirm={this.handleBulkDashboardDelete}
- >
- {confirmDelete => {
- const bulkActions = [];
- if (this.canDelete) {
- bulkActions.push({
- key: 'delete',
- name: (
- <>
- <i className="fa fa-trash" /> Delete
- </>
- ),
- onSelect: confirmDelete,
- });
- }
- if (this.canExport) {
- bulkActions.push({
- key: 'export',
- name: (
- <>
- <i className="fa fa-database" /> Export
- </>
- ),
- onSelect: this.handleBulkDashboardExport,
- });
- }
- return (
+ <>
+ <SubMenu name="Dashboards" />
Review comment:
nit: `t` string here please!
##########
File path: superset-frontend/src/views/datasetList/DatasetList.tsx
##########
@@ -193,36 +195,51 @@ class DatasetList extends React.PureComponent<Props, State> {
}: any) => kind[0]?.toUpperCase() + kind.slice(1),
Header: t('Type'),
accessor: 'kind',
+ size: 'lg',
},
{
Header: t('Source'),
accessor: 'database_name',
+ size: 'lg',
},
{
Header: t('Schema'),
accessor: 'schema',
+ size: 'lg',
},
{
Cell: ({
row: {
original: { changed_on: changedOn },
},
- }: any) => <span className="no-wrap">{moment(changedOn).fromNow()}</span>,
+ }: any) => {
+ const momentTime = moment(changedOn);
+ const time = momentTime.format('h:m a');
+ const date = momentTime.format('MMM D, YYYY');
Review comment:
it would be nice to move these format strings into constants (either in this file or in a more global constant file) assuming that these date and time formats will be widely used in the future
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r443762272
##########
File path: superset-frontend/src/components/Menu/SubMenu.tsx
##########
@@ -63,11 +63,10 @@ const StyledHeader = styled.header`
`;
interface Props {
- createButton: { name: string; url: string | null };
- canCreate: boolean;
- label: string;
+ createButton?: { name: string; url: string | null };
+ canCreate?: boolean;
name: string;
- childs: Array<{ label: string; name: string; url: string }>;
+ childs?: Array<{ label: string; name: string; url: string }>;
Review comment:
this is a legacy term from the FAB menu. I'll update
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
nytai merged pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r444471452
##########
File path: superset-frontend/src/components/AvatarIcon.tsx
##########
@@ -19,15 +19,16 @@
import React from 'react';
import styled from '@superset-ui/style';
import { getCategoricalSchemeRegistry } from '@superset-ui/color';
-import { Tooltip, OverlayTrigger } from 'react-bootstrap';
import Avatar, { ConfigProvider } from 'react-avatar';
+import TooltipWrapper from 'src/components/TooltipWrapper';
Review comment:
Just a comment that's out of scope for this PR, and a bit pedantic, but it might be nice to just call this component `Tooltip` since I suspect _most_ of our Superset components going forward will be wrappers for some react-bootstrap/antd/whateverLibrary component. I would just name the tool semantically, lest we start calling everything a Wrapper.
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r444492635
##########
File path: superset-frontend/src/components/ListView/ListViewStyles.less
##########
@@ -19,92 +19,127 @@
@import '~stylesheets/less/variables.less';
-.superset-list-view {
- .filter-dropdown {
- margin-top: 20px;
- }
+.superset-list-view-container {
+ text-align: center;
+
+ .superset-list-view {
+ text-align: left;
+ background-color: white;
+ border-radius: 4px 0;
+ overflow: scroll;
+ margin: 0 16px;
+ padding-bottom: 48px;
+
+ .filter-dropdown {
+ margin-top: 20px;
+ }
- .filter-column {
- height: 30px;
- padding: 5px;
- font-size: 16px;
- }
+ .filter-column {
+ height: 30px;
+ padding: 5px;
+ font-size: 16px;
+ }
- .filter-close {
- height: 30px;
- padding: 5px;
+ .filter-close {
+ height: 30px;
+ padding: 5px;
- i {
- font-size: 20px;
+ i {
+ font-size: 20px;
+ }
}
- }
- .table-row-loader {
- animation: shimmer 2s infinite;
- background: linear-gradient(
- to right,
- #f6f7f8 0%,
- #edeef1 20%,
- #f6f7f8 40%,
- #f6f7f8 100%
- );
- background-size: 1000px 100%;
-
- span {
- visibility: hidden;
+ .table-cell-loader {
+ position: relative;
+
+ .loading-bar {
+ background-color: #e7e7e7;
+ border-radius: 7px;
+
+ span {
+ visibility: hidden;
+ }
+ }
+
+ &:after {
+ position: absolute;
+ transform: translateY(-50%);
+ top: 50%;
+ left: 0;
+ content: '';
+ display: block;
+ width: 100%;
+ height: 48px;
+ background-image: linear-gradient(
+ 100deg,
+ rgba(255, 255, 255, 0),
+ rgba(255, 255, 255, 0.5) 60%,
+ rgba(255, 255, 255, 0) 80%
Review comment:
FWIW, the LESS variables have pre-baked colors and opacity stops, so you can do things like:
`background-color: fade(@success, @opacity-heavy);`
Those opacity stops haven't been migrated to the JS Theme for Emotion, yet. Emotion doesn't have the fancy color mixins, so we'd have to find the nifty JS way to accomplish the same goal.
----------------------------------------------------------------
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] mistercrunch commented on pull request #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#issuecomment-646701614
> The listview doesn't render until data is fetched and fetching new data uses a skeleton shimmer loading effect.
(probably outside the scope of this PR) we should discuss the ideal page states in minimizing flicker as the page load. To me I think the ideal flow would be:
1. show the page with a menu and list view shell first (with no data, but headers and layout in-place), I think that means we have to do that rendering in the `<HEAD/>` before the page is shown
1. and then show the data in the table as it comes (trigger the data fetch in the tail)
Assuming fixed width and a full page height, going from 1 to 2 would really just show the data in the existing cells, showing a total of 2 states.
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r443913240
##########
File path: superset-frontend/src/components/ListView/ListView.tsx
##########
@@ -116,124 +118,117 @@ const ListView: FunctionComponent<Props> = ({
}
});
}
-
+ if (loading && !data.length) {
+ return <Loading />;
+ }
return (
- <div className={`superset-list-view ${className}`}>
- <div className="header">
- {!useNewUIFilters && (
- <>
- {title && filterable && (
- <>
- <Row>
- <Col md={11}>
- <h2>{t(title)}</h2>
- </Col>
- {filterable && (
- <Col md={1}>
- <FilterMenu
- filters={filters}
- internalFilters={internalFilters}
- setInternalFilters={setInternalFilters}
- />
- </Col>
- )}
- </Row>
- <hr />
- <FilterInputs
- internalFilters={internalFilters}
- filters={filters}
- updateInternalFilter={updateInternalFilter}
- removeFilterAndApply={removeFilterAndApply}
- filtersApplied={filtersApplied}
- applyFilters={applyFilters}
- />
- </>
- )}
- </>
- )}
- {useNewUIFilters && (
- <>
- <Row>
- <Col md={10}>
- <h2>{t(title)}</h2>
- </Col>
- </Row>
- <hr />
+ <div className="superset-list-view-container">
+ <div className={`superset-list-view ${className}`}>
+ <div className="header">
+ {!useNewUIFilters && (
+ <>
+ {title && filterable && (
+ <>
+ <Row>
+ <Col md={10} />
+ {filterable && (
+ <Col md={2}>
+ <FilterMenu
+ filters={filters}
+ internalFilters={internalFilters}
+ setInternalFilters={setInternalFilters}
+ />
+ </Col>
+ )}
+ </Row>
+ <hr />
+ <FilterInputs
+ internalFilters={internalFilters}
+ filters={filters}
+ updateInternalFilter={updateInternalFilter}
+ removeFilterAndApply={removeFilterAndApply}
+ filtersApplied={filtersApplied}
+ applyFilters={applyFilters}
+ />
+ </>
+ )}
+ </>
+ )}
+ {useNewUIFilters && (
<FilterControls
filters={filters}
internalFilters={internalFilters}
updateFilterValue={applyFilterValue}
/>
- </>
- )}
- </div>
- <div className="body">
- <TableCollection
- getTableProps={getTableProps}
- getTableBodyProps={getTableBodyProps}
- prepareRow={prepareRow}
- headerGroups={headerGroups}
- rows={rows}
- loading={loading}
- />
- </div>
- <div className="footer">
- <Row>
- <Col md={2}>
- <div className="form-actions-container">
- <div className="btn-group">
- {bulkActions.length > 0 && (
- <DropdownButton
- id="bulk-actions"
- bsSize="small"
- bsStyle="default"
- noCaret
- title={
- <>
- {t('Actions')} <span className="caret" />
- </>
- }
- >
- {bulkActions.map(action => (
- // @ts-ignore
- <MenuItem
- key={action.key}
- eventKey={selectedFlatRows}
+ )}
+ </div>
+ <div className="body">
+ <TableCollection
+ getTableProps={getTableProps}
+ getTableBodyProps={getTableBodyProps}
+ prepareRow={prepareRow}
+ headerGroups={headerGroups}
+ rows={rows}
+ loading={loading}
+ />
+ </div>
+ <div className="footer">
+ <Row>
+ <Col>
+ <div className="form-actions-container">
+ <div className="btn-group">
+ {bulkActions.length > 0 && (
+ <DropdownButton
+ id="bulk-actions"
+ bsSize="small"
+ bsStyle="default"
+ noCaret
+ title={
+ <>
+ {t('Actions')} <span className="caret" />
+ </>
+ }
+ >
+ {bulkActions.map(action => (
// @ts-ignore
- onSelect={(selectedRows: typeof selectedFlatRows) => {
- action.onSelect(
- selectedRows.map((r: any) => r.original),
- );
- }}
- >
- {action.name}
- </MenuItem>
- ))}
- </DropdownButton>
- )}
+ <MenuItem
+ key={action.key}
+ eventKey={selectedFlatRows}
+ // @ts-ignore
+ onSelect={(selectedRows: typeof selectedFlatRows) => {
+ action.onSelect(
+ selectedRows.map((r: any) => r.original),
+ );
+ }}
+ >
+ {action.name}
+ </MenuItem>
+ ))}
+ </DropdownButton>
+ )}
+ </div>
</div>
- </div>
- </Col>
- <Col md={8} className="text-center">
- <Pagination
- totalPages={pageCount || 0}
- currentPage={pageCount ? pageIndex + 1 : 0}
- onChange={(p: number) => gotoPage(p - 1)}
- hideFirstAndLastPageLinks
- />
- </Col>
- <Col md={2}>
- <span className="pull-right">
- {t('showing')}{' '}
- <strong>
- {pageSize * pageIndex + (rows.length && 1)}-
- {pageSize * pageIndex + rows.length}
- </strong>{' '}
- {t('of')} <strong>{count}</strong>
- </span>
- </Col>
- </Row>
+ </Col>
+
+ <Col>
+ <span className="row-count-container">
+ {t('showing')}{' '}
+ <strong>
+ {pageSize * pageIndex + (rows.length && 1)}-
+ {pageSize * pageIndex + rows.length}
+ </strong>{' '}
+ {t('of')} <strong>{count}</strong>
Review comment:
removed the `t` strings. I've also asked design to look into it with translation in mind.
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r444383675
##########
File path: superset-frontend/src/components/AvatarIcon.tsx
##########
@@ -42,18 +43,26 @@ export default function AvatarIcon({
lastName,
userName,
iconSize,
+ textSize,
}: Props) {
const uniqueKey = `${tableName}-${userName}`;
const fullName = `${firstName} ${lastName}`;
return (
- <ConfigProvider colors={colorList && colorList.colors}>
- <OverlayTrigger
- placement="right"
- overlay={<Tooltip id={`${uniqueKey}-tooltip`}>{fullName}</Tooltip>}
- >
- <StyledAvatar key={uniqueKey} name={fullName} size={iconSize} round />
- </OverlayTrigger>
- </ConfigProvider>
+ <TooltipWrapper
+ placement="bottom"
+ label={`${uniqueKey}-tooltip`}
+ tooltip={fullName}
+ >
+ <ConfigProvider colors={colorList && colorList.colors}>
+ <StyledAvatar
+ key={uniqueKey}
+ name={fullName}
+ size={String(iconSize)}
+ textSizeRatio={Number(iconSize) / textSize}
Review comment:
Ah yes, looks like I had two competing approaches here.
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r444511579
##########
File path: superset-frontend/src/components/AvatarIcon.tsx
##########
@@ -19,15 +19,16 @@
import React from 'react';
import styled from '@superset-ui/style';
import { getCategoricalSchemeRegistry } from '@superset-ui/color';
-import { Tooltip, OverlayTrigger } from 'react-bootstrap';
import Avatar, { ConfigProvider } from 'react-avatar';
+import TooltipWrapper from 'src/components/TooltipWrapper';
Review comment:
The reason it's tooltipWrapper is because it just allows for any `tooltip` to be rendered with the correct triggers. `tooltip` is a prop of tooltipWrapper that defaults to using the bootstrap tooltip.
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r443762272
##########
File path: superset-frontend/src/components/Menu/SubMenu.tsx
##########
@@ -63,11 +63,10 @@ const StyledHeader = styled.header`
`;
interface Props {
- createButton: { name: string; url: string | null };
- canCreate: boolean;
- label: string;
+ createButton?: { name: string; url: string | null };
+ canCreate?: boolean;
name: string;
- childs: Array<{ label: string; name: string; url: string }>;
+ childs?: Array<{ label: string; name: string; url: string }>;
Review comment:
this is a legacy term from the FAB menu. I'll update
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r443914469
##########
File path: superset-frontend/src/components/ListView/ListViewStyles.less
##########
@@ -19,92 +19,127 @@
@import '~stylesheets/less/variables.less';
-.superset-list-view {
- .filter-dropdown {
- margin-top: 20px;
- }
+.superset-list-view-container {
+ text-align: center;
+
+ .superset-list-view {
+ text-align: left;
+ background-color: white;
+ border-radius: 4px 0;
+ overflow: scroll;
+ margin: 0 16px;
+ padding-bottom: 48px;
+
+ .filter-dropdown {
+ margin-top: 20px;
+ }
- .filter-column {
- height: 30px;
- padding: 5px;
- font-size: 16px;
- }
+ .filter-column {
+ height: 30px;
+ padding: 5px;
+ font-size: 16px;
+ }
- .filter-close {
- height: 30px;
- padding: 5px;
+ .filter-close {
+ height: 30px;
+ padding: 5px;
- i {
- font-size: 20px;
+ i {
+ font-size: 20px;
+ }
}
- }
- .table-row-loader {
- animation: shimmer 2s infinite;
- background: linear-gradient(
- to right,
- #f6f7f8 0%,
- #edeef1 20%,
- #f6f7f8 40%,
- #f6f7f8 100%
- );
- background-size: 1000px 100%;
-
- span {
- visibility: hidden;
+ .table-cell-loader {
+ position: relative;
+
+ .loading-bar {
+ background-color: #e7e7e7;
+ border-radius: 7px;
+
+ span {
+ visibility: hidden;
+ }
+ }
+
+ &:after {
+ position: absolute;
+ transform: translateY(-50%);
+ top: 50%;
+ left: 0;
+ content: '';
+ display: block;
+ width: 100%;
+ height: 48px;
+ background-image: linear-gradient(
+ 100deg,
+ rgba(255, 255, 255, 0),
+ rgba(255, 255, 255, 0.5) 60%,
+ rgba(255, 255, 255, 0) 80%
Review comment:
we don't have them and I don't see a huge advantage to making them variables; they're just black and black at half opacity.
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r442618749
##########
File path: superset-frontend/src/components/Menu/SubMenu.tsx
##########
@@ -63,11 +63,10 @@ const StyledHeader = styled.header`
`;
interface Props {
- createButton: { name: string; url: string | null };
- canCreate: boolean;
- label: string;
+ createButton?: { name: string; url: string | null };
+ canCreate?: boolean;
name: string;
- childs: Array<{ label: string; name: string; url: string }>;
+ childs?: Array<{ label: string; name: string; url: string }>;
Review comment:
children? ¯\\\_(ツ)_/¯
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r443914469
##########
File path: superset-frontend/src/components/ListView/ListViewStyles.less
##########
@@ -19,92 +19,127 @@
@import '~stylesheets/less/variables.less';
-.superset-list-view {
- .filter-dropdown {
- margin-top: 20px;
- }
+.superset-list-view-container {
+ text-align: center;
+
+ .superset-list-view {
+ text-align: left;
+ background-color: white;
+ border-radius: 4px 0;
+ overflow: scroll;
+ margin: 0 16px;
+ padding-bottom: 48px;
+
+ .filter-dropdown {
+ margin-top: 20px;
+ }
- .filter-column {
- height: 30px;
- padding: 5px;
- font-size: 16px;
- }
+ .filter-column {
+ height: 30px;
+ padding: 5px;
+ font-size: 16px;
+ }
- .filter-close {
- height: 30px;
- padding: 5px;
+ .filter-close {
+ height: 30px;
+ padding: 5px;
- i {
- font-size: 20px;
+ i {
+ font-size: 20px;
+ }
}
- }
- .table-row-loader {
- animation: shimmer 2s infinite;
- background: linear-gradient(
- to right,
- #f6f7f8 0%,
- #edeef1 20%,
- #f6f7f8 40%,
- #f6f7f8 100%
- );
- background-size: 1000px 100%;
-
- span {
- visibility: hidden;
+ .table-cell-loader {
+ position: relative;
+
+ .loading-bar {
+ background-color: #e7e7e7;
+ border-radius: 7px;
+
+ span {
+ visibility: hidden;
+ }
+ }
+
+ &:after {
+ position: absolute;
+ transform: translateY(-50%);
+ top: 50%;
+ left: 0;
+ content: '';
+ display: block;
+ width: 100%;
+ height: 48px;
+ background-image: linear-gradient(
+ 100deg,
+ rgba(255, 255, 255, 0),
+ rgba(255, 255, 255, 0.5) 60%,
+ rgba(255, 255, 255, 0) 80%
Review comment:
I'm not sure they need to be variables. It's just black and black at half opacity.
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r444477390
##########
File path: superset-frontend/src/components/ListView/ListView.tsx
##########
@@ -116,124 +116,111 @@ const ListView: FunctionComponent<Props> = ({
}
});
}
-
+ if (loading && !data.length) {
+ return <Loading />;
+ }
return (
- <div className={`superset-list-view ${className}`}>
- <div className="header">
- {!useNewUIFilters && (
- <>
- {title && filterable && (
- <>
- <Row>
- <Col md={11}>
- <h2>{t(title)}</h2>
- </Col>
- {filterable && (
- <Col md={1}>
- <FilterMenu
- filters={filters}
- internalFilters={internalFilters}
- setInternalFilters={setInternalFilters}
- />
- </Col>
- )}
- </Row>
- <hr />
- <FilterInputs
- internalFilters={internalFilters}
- filters={filters}
- updateInternalFilter={updateInternalFilter}
- removeFilterAndApply={removeFilterAndApply}
- filtersApplied={filtersApplied}
- applyFilters={applyFilters}
- />
- </>
- )}
- </>
- )}
- {useNewUIFilters && (
- <>
- <Row>
- <Col md={10}>
- <h2>{t(title)}</h2>
- </Col>
- </Row>
- <hr />
+ <div className="superset-list-view-container">
+ <div className={`superset-list-view ${className}`}>
+ <div className="header">
+ {!useNewUIFilters && filterable && (
+ <>
+ <Row>
+ <Col md={10} />
+ <Col md={2}>
+ <FilterMenu
+ filters={filters}
+ internalFilters={internalFilters}
+ setInternalFilters={setInternalFilters}
+ />
+ </Col>
+ </Row>
+ <hr />
+ <FilterInputs
+ internalFilters={internalFilters}
+ filters={filters}
+ updateInternalFilter={updateInternalFilter}
+ removeFilterAndApply={removeFilterAndApply}
+ filtersApplied={filtersApplied}
+ applyFilters={applyFilters}
+ />
+ </>
+ )}
+ {useNewUIFilters && filterable && (
<FilterControls
filters={filters}
internalFilters={internalFilters}
updateFilterValue={applyFilterValue}
/>
- </>
- )}
- </div>
- <div className="body">
- <TableCollection
- getTableProps={getTableProps}
- getTableBodyProps={getTableBodyProps}
- prepareRow={prepareRow}
- headerGroups={headerGroups}
- rows={rows}
- loading={loading}
- />
- </div>
- <div className="footer">
- <Row>
- <Col md={2}>
- <div className="form-actions-container">
- <div className="btn-group">
- {bulkActions.length > 0 && (
- <DropdownButton
- id="bulk-actions"
- bsSize="small"
- bsStyle="default"
- noCaret
- title={
- <>
- {t('Actions')} <span className="caret" />
- </>
- }
- >
- {bulkActions.map(action => (
- // @ts-ignore
- <MenuItem
- key={action.key}
- eventKey={selectedFlatRows}
+ )}
+ </div>
+ <div className="body">
+ <TableCollection
+ getTableProps={getTableProps}
+ getTableBodyProps={getTableBodyProps}
+ prepareRow={prepareRow}
+ headerGroups={headerGroups}
+ rows={rows}
+ loading={loading}
+ />
+ </div>
+ <div className="footer">
+ <Row>
+ <Col>
+ <div className="form-actions-container">
+ <div className="btn-group">
+ {bulkActions.length > 0 && (
+ <DropdownButton
+ id="bulk-actions"
+ bsSize="small"
+ bsStyle="default"
+ noCaret
+ title={
+ <>
+ {t('Actions')} <span className="caret" />
+ </>
+ }
+ >
+ {bulkActions.map(action => (
// @ts-ignore
- onSelect={(selectedRows: typeof selectedFlatRows) => {
- action.onSelect(
- selectedRows.map((r: any) => r.original),
- );
- }}
- >
- {action.name}
- </MenuItem>
- ))}
- </DropdownButton>
- )}
+ <MenuItem
+ key={action.key}
+ eventKey={selectedFlatRows}
+ // @ts-ignore
+ onSelect={(selectedRows: typeof selectedFlatRows) => {
+ action.onSelect(
+ selectedRows.map((r: any) => r.original),
+ );
+ }}
+ >
+ {action.name}
+ </MenuItem>
+ ))}
+ </DropdownButton>
+ )}
+ </div>
</div>
- </div>
- </Col>
- <Col md={8} className="text-center">
- <Pagination
- totalPages={pageCount || 0}
- currentPage={pageCount ? pageIndex + 1 : 0}
- onChange={(p: number) => gotoPage(p - 1)}
- hideFirstAndLastPageLinks
- />
- </Col>
- <Col md={2}>
- <span className="pull-right">
- {t('showing')}{' '}
- <strong>
- {pageSize * pageIndex + (rows.length && 1)}-
- {pageSize * pageIndex + rows.length}
- </strong>{' '}
- {t('of')} <strong>{count}</strong>
- </span>
- </Col>
- </Row>
+ </Col>
+
+ <Col>
+ <span className="row-count-container">
+ showing{' '}
Review comment:
Curious why "showing" and "of" were un-translated.
----------------------------------------------------------------
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 pull request #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
nytai commented on pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#issuecomment-646772668
@mistercrunch it would be nice to just render the skeleton and just have the data swap in when it arrives. In order to do this we need a good skeleton component that can be constructed to reasonably represent the table layout in question, or have dummy data that can take the shape of the data in question
----------------------------------------------------------------
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] john-bodley commented on pull request #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
john-bodley commented on pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#issuecomment-650431197
@nytai I believe this PR introduced a regression for the `/superset/welcome` page, i.e., per the attached screenshot the `showing 1-6 of 6` is floating outside the table.
<img width="1427" alt="Screen Shot 2020-06-26 at 3 09 45 PM" src="https://user-images.githubusercontent.com/4567245/85905804-4ad0f200-b7c1-11ea-9798-9c31907526d5.png">
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r442616368
##########
File path: superset-frontend/src/components/ListView/TableCollection.tsx
##########
@@ -29,6 +30,18 @@ interface Props {
rows: TableInstance['rows'];
loading: boolean;
}
+
+const TableCell = styled.td`
Review comment:
I see why you're adding `styled` here to apply props to the width, but there IS a possible performance implication here, if there wind up being a bajillion table cells, and Emotion starts bogging down. It may be more performant to use the prop to add a _class_ on the `td`, and then use emotion hire up in the DOM tree. I.e.
```
const Table = styled.table`
td{
&.xs { width: 25px; }
&.sm { width: 50px; }
&.md { width: 75px; }
&.lg { width: 100px; }
&.xl { width: 150px; }
&.xxl { width: 200px; }
}
`
```
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r442998604
##########
File path: superset-frontend/src/views/dashboardList/DashboardList.tsx
##########
@@ -508,71 +510,68 @@ class DashboardList extends React.PureComponent<Props, State> {
dashboardToEdit,
} = this.state;
return (
- <div className="container welcome">
- <Panel>
- <Panel.Body>
- <ConfirmStatusChange
- title={t('Please confirm')}
- description={t(
- 'Are you sure you want to delete the selected dashboards?',
- )}
- onConfirm={this.handleBulkDashboardDelete}
- >
- {confirmDelete => {
- const bulkActions = [];
- if (this.canDelete) {
- bulkActions.push({
- key: 'delete',
- name: (
- <>
- <i className="fa fa-trash" /> Delete
- </>
- ),
- onSelect: confirmDelete,
- });
- }
- if (this.canExport) {
- bulkActions.push({
- key: 'export',
- name: (
- <>
- <i className="fa fa-database" /> Export
- </>
- ),
- onSelect: this.handleBulkDashboardExport,
- });
- }
- return (
+ <>
Review comment:
Something that comes to mind as a flexible solution that would allow customization across the different views is a BaseListViewPage component that these could inherit from and override certain methods as needed. This would remove some of the code redundancies while allowing flexibility in overriding certain behavior. I generally try to avoid inheritance in JS when possible, but it seems like there's a case for it here. Thoughts?
Another approach might be to move this into hooks, ie `usePermissions`, `useFetchListData`, etc. though I'm not convinced this will really remove that much code redundancy
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r444510752
##########
File path: superset-frontend/src/components/ListView/ListView.tsx
##########
@@ -116,124 +116,111 @@ const ListView: FunctionComponent<Props> = ({
}
});
}
-
+ if (loading && !data.length) {
+ return <Loading />;
+ }
return (
- <div className={`superset-list-view ${className}`}>
- <div className="header">
- {!useNewUIFilters && (
- <>
- {title && filterable && (
- <>
- <Row>
- <Col md={11}>
- <h2>{t(title)}</h2>
- </Col>
- {filterable && (
- <Col md={1}>
- <FilterMenu
- filters={filters}
- internalFilters={internalFilters}
- setInternalFilters={setInternalFilters}
- />
- </Col>
- )}
- </Row>
- <hr />
- <FilterInputs
- internalFilters={internalFilters}
- filters={filters}
- updateInternalFilter={updateInternalFilter}
- removeFilterAndApply={removeFilterAndApply}
- filtersApplied={filtersApplied}
- applyFilters={applyFilters}
- />
- </>
- )}
- </>
- )}
- {useNewUIFilters && (
- <>
- <Row>
- <Col md={10}>
- <h2>{t(title)}</h2>
- </Col>
- </Row>
- <hr />
+ <div className="superset-list-view-container">
+ <div className={`superset-list-view ${className}`}>
+ <div className="header">
+ {!useNewUIFilters && filterable && (
+ <>
+ <Row>
+ <Col md={10} />
+ <Col md={2}>
+ <FilterMenu
+ filters={filters}
+ internalFilters={internalFilters}
+ setInternalFilters={setInternalFilters}
+ />
+ </Col>
+ </Row>
+ <hr />
+ <FilterInputs
+ internalFilters={internalFilters}
+ filters={filters}
+ updateInternalFilter={updateInternalFilter}
+ removeFilterAndApply={removeFilterAndApply}
+ filtersApplied={filtersApplied}
+ applyFilters={applyFilters}
+ />
+ </>
+ )}
+ {useNewUIFilters && filterable && (
<FilterControls
filters={filters}
internalFilters={internalFilters}
updateFilterValue={applyFilterValue}
/>
- </>
- )}
- </div>
- <div className="body">
- <TableCollection
- getTableProps={getTableProps}
- getTableBodyProps={getTableBodyProps}
- prepareRow={prepareRow}
- headerGroups={headerGroups}
- rows={rows}
- loading={loading}
- />
- </div>
- <div className="footer">
- <Row>
- <Col md={2}>
- <div className="form-actions-container">
- <div className="btn-group">
- {bulkActions.length > 0 && (
- <DropdownButton
- id="bulk-actions"
- bsSize="small"
- bsStyle="default"
- noCaret
- title={
- <>
- {t('Actions')} <span className="caret" />
- </>
- }
- >
- {bulkActions.map(action => (
- // @ts-ignore
- <MenuItem
- key={action.key}
- eventKey={selectedFlatRows}
+ )}
+ </div>
+ <div className="body">
+ <TableCollection
+ getTableProps={getTableProps}
+ getTableBodyProps={getTableBodyProps}
+ prepareRow={prepareRow}
+ headerGroups={headerGroups}
+ rows={rows}
+ loading={loading}
+ />
+ </div>
+ <div className="footer">
+ <Row>
+ <Col>
+ <div className="form-actions-container">
+ <div className="btn-group">
+ {bulkActions.length > 0 && (
+ <DropdownButton
+ id="bulk-actions"
+ bsSize="small"
+ bsStyle="default"
+ noCaret
+ title={
+ <>
+ {t('Actions')} <span className="caret" />
+ </>
+ }
+ >
+ {bulkActions.map(action => (
// @ts-ignore
- onSelect={(selectedRows: typeof selectedFlatRows) => {
- action.onSelect(
- selectedRows.map((r: any) => r.original),
- );
- }}
- >
- {action.name}
- </MenuItem>
- ))}
- </DropdownButton>
- )}
+ <MenuItem
+ key={action.key}
+ eventKey={selectedFlatRows}
+ // @ts-ignore
+ onSelect={(selectedRows: typeof selectedFlatRows) => {
+ action.onSelect(
+ selectedRows.map((r: any) => r.original),
+ );
+ }}
+ >
+ {action.name}
+ </MenuItem>
+ ))}
+ </DropdownButton>
+ )}
+ </div>
</div>
- </div>
- </Col>
- <Col md={8} className="text-center">
- <Pagination
- totalPages={pageCount || 0}
- currentPage={pageCount ? pageIndex + 1 : 0}
- onChange={(p: number) => gotoPage(p - 1)}
- hideFirstAndLastPageLinks
- />
- </Col>
- <Col md={2}>
- <span className="pull-right">
- {t('showing')}{' '}
- <strong>
- {pageSize * pageIndex + (rows.length && 1)}-
- {pageSize * pageIndex + rows.length}
- </strong>{' '}
- {t('of')} <strong>{count}</strong>
- </span>
- </Col>
- </Row>
+ </Col>
+
+ <Col>
+ <span className="row-count-container">
+ showing{' '}
Review comment:
this isn't a good way to translate strings. Not all languages follow the same structure as english, so just simply translating single words does not translate to a proper sentence in another language.
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r443911365
##########
File path: superset-frontend/src/components/Menu/SubMenu.tsx
##########
@@ -63,11 +63,10 @@ const StyledHeader = styled.header`
`;
interface Props {
- createButton: { name: string; url: string | null };
- canCreate: boolean;
- label: string;
+ createButton?: { name: string; url: string | null };
+ canCreate?: boolean;
name: string;
- childs: Array<{ label: string; name: string; url: string }>;
+ childs?: Array<{ label: string; name: string; url: string }>;
Review comment:
This is legacy terminology from FAB's menu structure and it's the same in the react menu component, https://github.com/preset-io/incubator-superset/blob/master/superset-frontend/src/components/Menu/MenuObject.jsx#L28
Also I think children will conflict with react's built in props
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r444510752
##########
File path: superset-frontend/src/components/ListView/ListView.tsx
##########
@@ -116,124 +116,111 @@ const ListView: FunctionComponent<Props> = ({
}
});
}
-
+ if (loading && !data.length) {
+ return <Loading />;
+ }
return (
- <div className={`superset-list-view ${className}`}>
- <div className="header">
- {!useNewUIFilters && (
- <>
- {title && filterable && (
- <>
- <Row>
- <Col md={11}>
- <h2>{t(title)}</h2>
- </Col>
- {filterable && (
- <Col md={1}>
- <FilterMenu
- filters={filters}
- internalFilters={internalFilters}
- setInternalFilters={setInternalFilters}
- />
- </Col>
- )}
- </Row>
- <hr />
- <FilterInputs
- internalFilters={internalFilters}
- filters={filters}
- updateInternalFilter={updateInternalFilter}
- removeFilterAndApply={removeFilterAndApply}
- filtersApplied={filtersApplied}
- applyFilters={applyFilters}
- />
- </>
- )}
- </>
- )}
- {useNewUIFilters && (
- <>
- <Row>
- <Col md={10}>
- <h2>{t(title)}</h2>
- </Col>
- </Row>
- <hr />
+ <div className="superset-list-view-container">
+ <div className={`superset-list-view ${className}`}>
+ <div className="header">
+ {!useNewUIFilters && filterable && (
+ <>
+ <Row>
+ <Col md={10} />
+ <Col md={2}>
+ <FilterMenu
+ filters={filters}
+ internalFilters={internalFilters}
+ setInternalFilters={setInternalFilters}
+ />
+ </Col>
+ </Row>
+ <hr />
+ <FilterInputs
+ internalFilters={internalFilters}
+ filters={filters}
+ updateInternalFilter={updateInternalFilter}
+ removeFilterAndApply={removeFilterAndApply}
+ filtersApplied={filtersApplied}
+ applyFilters={applyFilters}
+ />
+ </>
+ )}
+ {useNewUIFilters && filterable && (
<FilterControls
filters={filters}
internalFilters={internalFilters}
updateFilterValue={applyFilterValue}
/>
- </>
- )}
- </div>
- <div className="body">
- <TableCollection
- getTableProps={getTableProps}
- getTableBodyProps={getTableBodyProps}
- prepareRow={prepareRow}
- headerGroups={headerGroups}
- rows={rows}
- loading={loading}
- />
- </div>
- <div className="footer">
- <Row>
- <Col md={2}>
- <div className="form-actions-container">
- <div className="btn-group">
- {bulkActions.length > 0 && (
- <DropdownButton
- id="bulk-actions"
- bsSize="small"
- bsStyle="default"
- noCaret
- title={
- <>
- {t('Actions')} <span className="caret" />
- </>
- }
- >
- {bulkActions.map(action => (
- // @ts-ignore
- <MenuItem
- key={action.key}
- eventKey={selectedFlatRows}
+ )}
+ </div>
+ <div className="body">
+ <TableCollection
+ getTableProps={getTableProps}
+ getTableBodyProps={getTableBodyProps}
+ prepareRow={prepareRow}
+ headerGroups={headerGroups}
+ rows={rows}
+ loading={loading}
+ />
+ </div>
+ <div className="footer">
+ <Row>
+ <Col>
+ <div className="form-actions-container">
+ <div className="btn-group">
+ {bulkActions.length > 0 && (
+ <DropdownButton
+ id="bulk-actions"
+ bsSize="small"
+ bsStyle="default"
+ noCaret
+ title={
+ <>
+ {t('Actions')} <span className="caret" />
+ </>
+ }
+ >
+ {bulkActions.map(action => (
// @ts-ignore
- onSelect={(selectedRows: typeof selectedFlatRows) => {
- action.onSelect(
- selectedRows.map((r: any) => r.original),
- );
- }}
- >
- {action.name}
- </MenuItem>
- ))}
- </DropdownButton>
- )}
+ <MenuItem
+ key={action.key}
+ eventKey={selectedFlatRows}
+ // @ts-ignore
+ onSelect={(selectedRows: typeof selectedFlatRows) => {
+ action.onSelect(
+ selectedRows.map((r: any) => r.original),
+ );
+ }}
+ >
+ {action.name}
+ </MenuItem>
+ ))}
+ </DropdownButton>
+ )}
+ </div>
</div>
- </div>
- </Col>
- <Col md={8} className="text-center">
- <Pagination
- totalPages={pageCount || 0}
- currentPage={pageCount ? pageIndex + 1 : 0}
- onChange={(p: number) => gotoPage(p - 1)}
- hideFirstAndLastPageLinks
- />
- </Col>
- <Col md={2}>
- <span className="pull-right">
- {t('showing')}{' '}
- <strong>
- {pageSize * pageIndex + (rows.length && 1)}-
- {pageSize * pageIndex + rows.length}
- </strong>{' '}
- {t('of')} <strong>{count}</strong>
- </span>
- </Col>
- </Row>
+ </Col>
+
+ <Col>
+ <span className="row-count-container">
+ showing{' '}
Review comment:
this isn't a good way to translate strings. Not all languages follow the same structure as english, so just simply translating single words does not translate to proper sentence in another language.
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r442949081
##########
File path: superset-frontend/src/components/ListView/ListView.tsx
##########
@@ -116,124 +118,117 @@ const ListView: FunctionComponent<Props> = ({
}
});
}
-
+ if (loading && !data.length) {
+ return <Loading />;
+ }
return (
- <div className={`superset-list-view ${className}`}>
- <div className="header">
- {!useNewUIFilters && (
- <>
- {title && filterable && (
- <>
- <Row>
- <Col md={11}>
- <h2>{t(title)}</h2>
- </Col>
- {filterable && (
- <Col md={1}>
- <FilterMenu
- filters={filters}
- internalFilters={internalFilters}
- setInternalFilters={setInternalFilters}
- />
- </Col>
- )}
- </Row>
- <hr />
- <FilterInputs
- internalFilters={internalFilters}
- filters={filters}
- updateInternalFilter={updateInternalFilter}
- removeFilterAndApply={removeFilterAndApply}
- filtersApplied={filtersApplied}
- applyFilters={applyFilters}
- />
- </>
- )}
- </>
- )}
- {useNewUIFilters && (
- <>
- <Row>
- <Col md={10}>
- <h2>{t(title)}</h2>
- </Col>
- </Row>
- <hr />
+ <div className="superset-list-view-container">
+ <div className={`superset-list-view ${className}`}>
+ <div className="header">
+ {!useNewUIFilters && (
+ <>
+ {title && filterable && (
+ <>
+ <Row>
+ <Col md={10} />
+ {filterable && (
+ <Col md={2}>
+ <FilterMenu
+ filters={filters}
+ internalFilters={internalFilters}
+ setInternalFilters={setInternalFilters}
+ />
+ </Col>
+ )}
+ </Row>
+ <hr />
+ <FilterInputs
+ internalFilters={internalFilters}
+ filters={filters}
+ updateInternalFilter={updateInternalFilter}
+ removeFilterAndApply={removeFilterAndApply}
+ filtersApplied={filtersApplied}
+ applyFilters={applyFilters}
+ />
+ </>
+ )}
+ </>
+ )}
+ {useNewUIFilters && (
<FilterControls
filters={filters}
internalFilters={internalFilters}
updateFilterValue={applyFilterValue}
/>
- </>
- )}
- </div>
- <div className="body">
- <TableCollection
- getTableProps={getTableProps}
- getTableBodyProps={getTableBodyProps}
- prepareRow={prepareRow}
- headerGroups={headerGroups}
- rows={rows}
- loading={loading}
- />
- </div>
- <div className="footer">
- <Row>
- <Col md={2}>
- <div className="form-actions-container">
- <div className="btn-group">
- {bulkActions.length > 0 && (
- <DropdownButton
- id="bulk-actions"
- bsSize="small"
- bsStyle="default"
- noCaret
- title={
- <>
- {t('Actions')} <span className="caret" />
- </>
- }
- >
- {bulkActions.map(action => (
- // @ts-ignore
- <MenuItem
- key={action.key}
- eventKey={selectedFlatRows}
+ )}
+ </div>
+ <div className="body">
+ <TableCollection
+ getTableProps={getTableProps}
+ getTableBodyProps={getTableBodyProps}
+ prepareRow={prepareRow}
+ headerGroups={headerGroups}
+ rows={rows}
+ loading={loading}
+ />
+ </div>
+ <div className="footer">
+ <Row>
+ <Col>
+ <div className="form-actions-container">
+ <div className="btn-group">
+ {bulkActions.length > 0 && (
+ <DropdownButton
+ id="bulk-actions"
+ bsSize="small"
+ bsStyle="default"
+ noCaret
+ title={
+ <>
+ {t('Actions')} <span className="caret" />
+ </>
+ }
+ >
+ {bulkActions.map(action => (
// @ts-ignore
- onSelect={(selectedRows: typeof selectedFlatRows) => {
- action.onSelect(
- selectedRows.map((r: any) => r.original),
- );
- }}
- >
- {action.name}
- </MenuItem>
- ))}
- </DropdownButton>
- )}
+ <MenuItem
+ key={action.key}
+ eventKey={selectedFlatRows}
+ // @ts-ignore
+ onSelect={(selectedRows: typeof selectedFlatRows) => {
+ action.onSelect(
+ selectedRows.map((r: any) => r.original),
+ );
+ }}
+ >
+ {action.name}
+ </MenuItem>
+ ))}
+ </DropdownButton>
+ )}
+ </div>
</div>
- </div>
- </Col>
- <Col md={8} className="text-center">
- <Pagination
- totalPages={pageCount || 0}
- currentPage={pageCount ? pageIndex + 1 : 0}
- onChange={(p: number) => gotoPage(p - 1)}
- hideFirstAndLastPageLinks
- />
- </Col>
- <Col md={2}>
- <span className="pull-right">
- {t('showing')}{' '}
- <strong>
- {pageSize * pageIndex + (rows.length && 1)}-
- {pageSize * pageIndex + rows.length}
- </strong>{' '}
- {t('of')} <strong>{count}</strong>
- </span>
- </Col>
- </Row>
+ </Col>
+
+ <Col>
+ <span className="row-count-container">
+ {t('showing')}{' '}
+ <strong>
+ {pageSize * pageIndex + (rows.length && 1)}-
+ {pageSize * pageIndex + rows.length}
+ </strong>{' '}
+ {t('of')} <strong>{count}</strong>
Review comment:
@etr2460 I wrote this before I really understood how translation strings worked. Any idea how to get the html (`<strong>`) interspersed in the translation string?
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r442996744
##########
File path: superset-frontend/src/components/ListView/ListView.tsx
##########
@@ -116,124 +118,117 @@ const ListView: FunctionComponent<Props> = ({
}
});
}
-
+ if (loading && !data.length) {
+ return <Loading />;
+ }
return (
- <div className={`superset-list-view ${className}`}>
- <div className="header">
- {!useNewUIFilters && (
- <>
- {title && filterable && (
- <>
- <Row>
- <Col md={11}>
- <h2>{t(title)}</h2>
- </Col>
- {filterable && (
- <Col md={1}>
- <FilterMenu
- filters={filters}
- internalFilters={internalFilters}
- setInternalFilters={setInternalFilters}
- />
- </Col>
- )}
- </Row>
- <hr />
- <FilterInputs
- internalFilters={internalFilters}
- filters={filters}
- updateInternalFilter={updateInternalFilter}
- removeFilterAndApply={removeFilterAndApply}
- filtersApplied={filtersApplied}
- applyFilters={applyFilters}
- />
- </>
- )}
- </>
- )}
- {useNewUIFilters && (
- <>
- <Row>
- <Col md={10}>
- <h2>{t(title)}</h2>
- </Col>
- </Row>
- <hr />
+ <div className="superset-list-view-container">
+ <div className={`superset-list-view ${className}`}>
+ <div className="header">
+ {!useNewUIFilters && (
+ <>
+ {title && filterable && (
+ <>
+ <Row>
+ <Col md={10} />
+ {filterable && (
+ <Col md={2}>
+ <FilterMenu
+ filters={filters}
+ internalFilters={internalFilters}
+ setInternalFilters={setInternalFilters}
+ />
+ </Col>
+ )}
+ </Row>
+ <hr />
+ <FilterInputs
+ internalFilters={internalFilters}
+ filters={filters}
+ updateInternalFilter={updateInternalFilter}
+ removeFilterAndApply={removeFilterAndApply}
+ filtersApplied={filtersApplied}
+ applyFilters={applyFilters}
+ />
+ </>
+ )}
+ </>
+ )}
+ {useNewUIFilters && (
<FilterControls
filters={filters}
internalFilters={internalFilters}
updateFilterValue={applyFilterValue}
/>
- </>
- )}
- </div>
- <div className="body">
- <TableCollection
- getTableProps={getTableProps}
- getTableBodyProps={getTableBodyProps}
- prepareRow={prepareRow}
- headerGroups={headerGroups}
- rows={rows}
- loading={loading}
- />
- </div>
- <div className="footer">
- <Row>
- <Col md={2}>
- <div className="form-actions-container">
- <div className="btn-group">
- {bulkActions.length > 0 && (
- <DropdownButton
- id="bulk-actions"
- bsSize="small"
- bsStyle="default"
- noCaret
- title={
- <>
- {t('Actions')} <span className="caret" />
- </>
- }
- >
- {bulkActions.map(action => (
- // @ts-ignore
- <MenuItem
- key={action.key}
- eventKey={selectedFlatRows}
+ )}
+ </div>
+ <div className="body">
+ <TableCollection
+ getTableProps={getTableProps}
+ getTableBodyProps={getTableBodyProps}
+ prepareRow={prepareRow}
+ headerGroups={headerGroups}
+ rows={rows}
+ loading={loading}
+ />
+ </div>
+ <div className="footer">
+ <Row>
+ <Col>
+ <div className="form-actions-container">
+ <div className="btn-group">
+ {bulkActions.length > 0 && (
+ <DropdownButton
+ id="bulk-actions"
+ bsSize="small"
+ bsStyle="default"
+ noCaret
+ title={
+ <>
+ {t('Actions')} <span className="caret" />
+ </>
+ }
+ >
+ {bulkActions.map(action => (
// @ts-ignore
- onSelect={(selectedRows: typeof selectedFlatRows) => {
- action.onSelect(
- selectedRows.map((r: any) => r.original),
- );
- }}
- >
- {action.name}
- </MenuItem>
- ))}
- </DropdownButton>
- )}
+ <MenuItem
+ key={action.key}
+ eventKey={selectedFlatRows}
+ // @ts-ignore
+ onSelect={(selectedRows: typeof selectedFlatRows) => {
+ action.onSelect(
+ selectedRows.map((r: any) => r.original),
+ );
+ }}
+ >
+ {action.name}
+ </MenuItem>
+ ))}
+ </DropdownButton>
+ )}
+ </div>
</div>
- </div>
- </Col>
- <Col md={8} className="text-center">
- <Pagination
- totalPages={pageCount || 0}
- currentPage={pageCount ? pageIndex + 1 : 0}
- onChange={(p: number) => gotoPage(p - 1)}
- hideFirstAndLastPageLinks
- />
- </Col>
- <Col md={2}>
- <span className="pull-right">
- {t('showing')}{' '}
- <strong>
- {pageSize * pageIndex + (rows.length && 1)}-
- {pageSize * pageIndex + rows.length}
- </strong>{' '}
- {t('of')} <strong>{count}</strong>
- </span>
- </Col>
- </Row>
+ </Col>
+
+ <Col>
+ <span className="row-count-container">
+ {t('showing')}{' '}
+ <strong>
+ {pageSize * pageIndex + (rows.length && 1)}-
+ {pageSize * pageIndex + rows.length}
+ </strong>{' '}
+ {t('of')} <strong>{count}</strong>
Review comment:
ah, it's not possible currently. I know Max was trying to figure out a way to hack around it, but I don't think we ever came up with a solution.
If the bold styling is necessary here, then i'd say remove the `t` strings entirely. If not, then it might be better to remove the styling for now to support better translations
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r442953528
##########
File path: superset-frontend/src/views/dashboardList/DashboardList.tsx
##########
@@ -508,71 +510,68 @@ class DashboardList extends React.PureComponent<Props, State> {
dashboardToEdit,
} = this.state;
return (
- <div className="container welcome">
- <Panel>
- <Panel.Body>
- <ConfirmStatusChange
- title={t('Please confirm')}
- description={t(
- 'Are you sure you want to delete the selected dashboards?',
- )}
- onConfirm={this.handleBulkDashboardDelete}
- >
- {confirmDelete => {
- const bulkActions = [];
- if (this.canDelete) {
- bulkActions.push({
- key: 'delete',
- name: (
- <>
- <i className="fa fa-trash" /> Delete
- </>
- ),
- onSelect: confirmDelete,
- });
- }
- if (this.canExport) {
- bulkActions.push({
- key: 'export',
- name: (
- <>
- <i className="fa fa-database" /> Export
- </>
- ),
- onSelect: this.handleBulkDashboardExport,
- });
- }
- return (
+ <>
Review comment:
this is where it gets tricky, the code is largely the same but there are a few differences that make it difficult to isolate to a single component. It also couples the code so that changes in one view could result in unexpected changes in another view. I am planning to revisit this question once some of the design on these listviews has settled and is not likely to change drastically.
----------------------------------------------------------------
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] mistercrunch edited a comment on pull request #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
mistercrunch edited a comment on pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#issuecomment-646701614
> The listview doesn't render until data is fetched and fetching new data uses a skeleton shimmer loading effect.
(probably outside the scope of this PR) we should discuss the ideal page states in minimizing flicker as the page load. To me I think the ideal flow would be:
1. show the page with a navbar and list view shell first (with no data, but headers and layout in-place), I think that means we have to do that rendering in the `<HEAD/>` before the page is shown
1. and then show the data in the table as it comes (trigger the data fetch in the tail)
Assuming fixed width and a full page height, going from 1 to 2 would really just show the data in the existing cells, showing a total of 2 states.
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r444472545
##########
File path: superset-frontend/src/components/AvatarIcon.tsx
##########
@@ -19,15 +19,16 @@
import React from 'react';
import styled from '@superset-ui/style';
import { getCategoricalSchemeRegistry } from '@superset-ui/color';
-import { Tooltip, OverlayTrigger } from 'react-bootstrap';
import Avatar, { ConfigProvider } from 'react-avatar';
+import TooltipWrapper from 'src/components/TooltipWrapper';
Review comment:
... and if this was due to namespacing issues, I would just rename the internal import, e.g. `import { Tooltip as RB_Tooltip } from 'react-bootstrap';
----------------------------------------------------------------
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] mistercrunch commented on pull request #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#issuecomment-648502754
----------------------------------------------------------------
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 #10094: style: listviews closer to SIP-34
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10094:
URL: https://github.com/apache/incubator-superset/pull/10094#discussion_r442974007
##########
File path: superset-frontend/src/components/Pagination.tsx
##########
@@ -0,0 +1,132 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { PureComponent } from 'react';
+import cx from 'classnames';
+import styled from '@superset-ui/style';
+
+interface PaginationButton {
+ disabled?: boolean;
+ onClick: React.EventHandler<React.SyntheticEvent<HTMLElement>>;
+}
+
+interface PaginationItemButton extends PaginationButton {
+ active: boolean;
+ children: React.ReactNode;
+}
+
+function Prev({ disabled, onClick }: PaginationButton) {
+ return (
+ <li className={cx({ disabled })}>
+ <span role="button" tabIndex={disabled ? -1 : 0} onClick={onClick}>
+ «
Review comment:
the mocks we have for SIP-34 uses unicode in these components :)
----------------------------------------------------------------
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