rusackas commented on code in PR #39461:
URL: https://github.com/apache/superset/pull/39461#discussion_r3229990162
##########
superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx:
##########
@@ -407,103 +431,101 @@ export default class CRUDCollection extends
PureComponent<
});
}
- return antdColumns as ColumnsType<CollectionItem>;
- }
-
- render() {
- const {
- stickyHeader,
- emptyMessage = t('No items'),
- expandFieldset,
- pagination = false,
- filterTerm,
- filterFields,
- } = this.props;
-
- const displayData =
- filterTerm && filterFields?.length
- ? this.state.collectionArray.filter(item =>
- filterFields.some(field =>
- String(item[field] ?? '')
- .toLowerCase()
- .includes(filterTerm.toLowerCase()),
- ),
- )
- : this.state.collectionArray;
-
- const tableColumns = this.buildTableColumns();
- const expandedRowKeys = Object.keys(this.state.expandedColumns).filter(
- id => this.state.expandedColumns[id],
- );
-
- const expandableConfig = expandFieldset
- ? {
- expandedRowRender: (record: CollectionItem) =>
- this.renderExpandableSection(record),
- rowExpandable: () => true,
- expandedRowKeys,
- onExpand: (expanded: boolean, record: CollectionItem) => {
- this.toggleExpand(record.id);
- },
- }
- : undefined;
-
- // Build controlled pagination config, clamping currentPage to valid range
- // based on displayData (filtered) length, not the full collection
- const { pageSize, currentPage: statePage } = this.state;
- const totalItems = displayData.length;
- const maxPage = totalItems > 0 ? Math.ceil(totalItems / pageSize) : 1;
- const currentPage = Math.min(statePage, maxPage);
- const paginationConfig: false | TablePaginationConfig | undefined =
- pagination === false || pagination === undefined
- ? pagination
- : {
- ...(typeof pagination === 'object' ? pagination : {}),
- current: currentPage,
- pageSize,
- total: totalItems,
- };
+ return columns;
+ }, [
+ tableColumns,
+ getLabel,
+ getTooltip,
+ sortColumns,
+ sortColumn,
+ sort,
+ renderCell,
+ itemCellProps,
+ allowDeletes,
+ deleteItem,
+ ]);
+
+ const displayData = useMemo(() => {
+ if (filterTerm && filterFields?.length) {
+ return collectionArray.filter(item =>
+ filterFields.some(field =>
+ String(item[field] ?? '')
+ .toLowerCase()
+ .includes(filterTerm.toLowerCase()),
+ ),
+ );
+ }
+ return collectionArray;
+ }, [collectionArray, filterTerm, filterFields]);
- return (
- <>
- <CrudButtonWrapper>
- {this.props.allowAddItem && (
- <StyledButtonWrapper>
- <Button
- buttonSize="small"
- buttonStyle="secondary"
- onClick={this.onAddItem}
- data-test="add-item-button"
- >
- <Icons.PlusOutlined
- iconSize="m"
- data-test="crud-add-table-item"
- />
- {t('Add item')}
- </Button>
- </StyledButtonWrapper>
- )}
- </CrudButtonWrapper>
- <Table<CollectionItem>
- data-test="crud-table"
- columns={tableColumns}
- data={displayData as CollectionItem[]}
- rowKey={(record: CollectionItem) => String(record.id)}
- sticky={stickyHeader}
- pagination={paginationConfig}
- onChange={this.handleTableChange}
- locale={{ emptyText: emptyMessage }}
- css={
- stickyHeader &&
- css`
- overflow: auto;
- `
+ const paginationConfig = useMemo((): false | TablePaginationConfig => {
+ if (pagination === false || pagination === undefined) {
+ return false;
+ }
+ return typeof pagination === 'object' ? pagination : {};
+ }, [pagination]);
Review Comment:
Done in a01a855603 — restored controlled `currentPage`/`pageSize` state,
updated `handleTableChange` to track antd's pagination events, and built a
`paginationConfig` that clamps `current` to `Math.ceil(filteredLength /
pageSize)` on every render. Filtering down to fewer rows now lands the user on
a valid page instead of an empty one.
##########
superset-frontend/src/components/CopyToClipboard/index.tsx:
##########
@@ -16,126 +16,137 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { Component, cloneElement, ReactElement } from 'react';
+import { cloneElement, isValidElement, ReactElement, useCallback } from
'react';
import { t } from '@apache-superset/core/translation';
import { css, SupersetTheme } from '@apache-superset/core/theme';
import copyTextToClipboard from 'src/utils/copy';
import { Tooltip } from '@superset-ui/core/components';
import withToasts from '../MessageToasts/withToasts';
import type { CopyToClipboardProps } from './types';
-const defaultProps: Partial<CopyToClipboardProps> = {
- copyNode: <span>{t('Copy')}</span>,
- onCopyEnd: () => {},
- shouldShowText: true,
- wrapped: true,
- tooltipText: t('Copy to clipboard'),
- hideTooltip: false,
-};
+function CopyToClip({
+ copyNode = <span>{t('Copy')}</span>,
+ onCopyEnd = () => {},
+ shouldShowText = true,
+ wrapped = true,
+ tooltipText = t('Copy to clipboard'),
+ hideTooltip = false,
+ disabled,
+ getText,
+ text,
+ addSuccessToast,
+ addDangerToast,
+}: CopyToClipboardProps) {
+ const copyToClipboard = useCallback(
+ (textToCopy: Promise<string>) => {
+ copyTextToClipboard(() => textToCopy)
+ .then(() => {
+ addSuccessToast(t('Copied to clipboard!'));
+ })
+ .catch(() => {
+ addDangerToast(
+ t(
+ 'Sorry, your browser does not support copying. Use Ctrl / Cmd +
C!',
+ ),
+ );
+ })
+ .finally(() => {
+ if (onCopyEnd) onCopyEnd();
+ });
+ },
+ [addSuccessToast, addDangerToast, onCopyEnd],
+ );
-class CopyToClip extends Component<CopyToClipboardProps> {
- static defaultProps = defaultProps;
-
- constructor(props: CopyToClipboardProps) {
- super(props);
- this.copyToClipboard = this.copyToClipboard.bind(this);
- this.onClick = this.onClick.bind(this);
- }
-
- onClick() {
- if (this.props.disabled) {
+ const onClick = useCallback(() => {
+ if (disabled) {
return;
}
- if (this.props.getText) {
- this.props.getText((d: string) => {
- this.copyToClipboard(Promise.resolve(d));
+ if (getText) {
+ getText((d: string) => {
+ copyToClipboard(Promise.resolve(d));
});
} else {
- this.copyToClipboard(Promise.resolve(this.props.text || ''));
+ copyToClipboard(Promise.resolve(text || ''));
}
- }
-
- getDecoratedCopyNode() {
- const copyNode = this.props.copyNode as ReactElement;
- const { disabled } = this.props;
- return cloneElement(copyNode, {
- style: {
- ...copyNode.props.style,
- cursor: disabled ? 'not-allowed' : 'pointer',
- },
- onClick: disabled ? undefined : this.onClick,
- 'aria-disabled': disabled || undefined,
- tabIndex: disabled ? -1 : copyNode.props.tabIndex,
- });
- }
+ }, [disabled, getText, text, copyToClipboard]);
- copyToClipboard(textToCopy: Promise<string>) {
- copyTextToClipboard(() => textToCopy)
- .then(() => {
- this.props.addSuccessToast(t('Copied to clipboard!'));
- })
- .catch(() => {
- this.props.addDangerToast(
- t(
- 'Sorry, your browser does not support copying. Use Ctrl / Cmd +
C!',
- ),
- );
- })
- .finally(() => {
- if (this.props.onCopyEnd) this.props.onCopyEnd();
+ const getDecoratedCopyNode = useCallback(() => {
+ const cursor = disabled ? 'not-allowed' : 'pointer';
+ if (isValidElement(copyNode)) {
+ const node = copyNode as ReactElement;
+ return cloneElement(node, {
+ style: {
+ ...node.props.style,
+ cursor,
+ },
+ onClick: disabled ? undefined : onClick,
+ 'aria-disabled': disabled || undefined,
+ tabIndex: disabled ? -1 : node.props.tabIndex,
});
- }
-
- renderTooltip(cursor: string) {
+ }
return (
+ <span
+ style={{ cursor }}
+ onClick={disabled ? undefined : onClick}
+ role="button"
+ aria-disabled={disabled || undefined}
+ tabIndex={disabled ? -1 : undefined}
+ >
+ {copyNode}
+ </span>
+ );
Review Comment:
Done in f021cf2060 — added an `onKeyDown` handler that fires `onClick` on
Enter or Space (with `preventDefault` to suppress space-scroll), and set
`tabIndex={0}` by default so the wrapper is in tab order. Added a regression
test that focuses the wrapper and asserts Enter triggers `onCopyEnd`.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
