msyavuz commented on code in PR #32705:
URL: https://github.com/apache/superset/pull/32705#discussion_r2020682946
##########
superset-frontend/src/components/Layout/index.tsx:
##########
@@ -17,5 +17,7 @@
* under the License.
*/
import { Layout } from 'antd-v5';
+import type { LayoutProps, SiderProps } from 'antd-v5/es/layout';
+export type { LayoutProps, SiderProps };
export default Layout;
Review Comment:
We can change this to named exports as well
##########
superset-frontend/src/components/Select/AsyncSelect.stories.tsx:
##########
@@ -18,12 +18,12 @@
*/
import { ReactNode, useState, useCallback, useRef, useMemo } from 'react';
import Button from 'src/components/Button';
-import AsyncSelect from './AsyncSelect';
import {
AsyncSelectProps,
AsyncSelectRef,
SelectOptionsTypePage,
-} from './types';
+} from 'src/components/Select/types';
+import { AsyncSelect } from '.';
Review Comment:
Let's stick to one import either relative `./` or from `src/components`
##########
superset-frontend/src/components/Typography/index.tsx:
##########
@@ -0,0 +1,21 @@
+/**
+ * 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 { Typography } from 'antd-v5';
+
+export default Typography;
Review Comment:
Same here
##########
superset-frontend/src/components/Tree/index.tsx:
##########
@@ -0,0 +1,25 @@
+/**
+ * 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 Tree from 'antd-v5/lib/tree/Tree';
+import type { TreeProps, DataNode as TreeDataNode } from 'antd-v5/es/tree';
+import DirectoryTree from 'antd-v5/lib/tree/DirectoryTree';
+
+export type { TreeProps };
+export { TreeDataNode, DirectoryTree };
+export default Tree;
Review Comment:
Just keeping this unresolved as a reminder for named exports
##########
superset-frontend/src/components/Chart/DrillDetail/DrillDetailTableControls.test.tsx:
##########
@@ -101,6 +101,6 @@ test('should remove the filters on close', () => {
expect(screen.getByText('platform')).toBeInTheDocument();
expect(screen.getByText('GB')).toBeInTheDocument();
- userEvent.click(screen.getByRole('button', { name: 'close' }));
+ userEvent.click(screen.getByRole('img', { name: 'close' }));
Review Comment:
Do we know why this change is necessary? I suspect this means close button
is not accessible now.
##########
superset-frontend/src/components/Timer/Timer.test.tsx:
##########
@@ -53,17 +58,17 @@ describe('Timer', () => {
screen.rerender(<Timer {...mockProps} isRunning={false} />);
// the same node should still be in DOM and the content should not change
expect(screen.getByRole('timer')).toBe(node);
- expect(node).toHaveTextContent(text);
+ expect(formatTime(node.textContent || '')).toBe(formatTime(text));
// the timestamp should not change even after while
await sleep(100);
expect(screen.getByRole('timer')).toBe(node);
- expect(node).toHaveTextContent(text);
+ expect(formatTime(node.textContent || '')).toBe(formatTime(text));
// should continue and start from stopped time
screen.rerender(<Timer {...mockProps} isRunning />);
expect(screen.getByRole('timer')).toBe(node);
- expect(node).toHaveTextContent(text);
+ expect(formatTime(node.textContent || '')).toBe(formatTime(text));
Review Comment:
I think only the change in `Label` component might necessitate this but the
styles look the same there so wondering why Timer needs to have different text?
##########
superset-frontend/src/components/Skeleton/index.tsx:
##########
@@ -16,6 +16,6 @@
* specific language governing permissions and limitations
* under the License.
*/
+import { Skeleton } from 'antd-v5';
-export { default as TagsList } from './TagsList';
-export { default as Tag } from './Tag';
+export default Skeleton;
Review Comment:
We can change this to named export as well
##########
superset-frontend/src/components/TreeSelect/index.tsx:
##########
@@ -0,0 +1,23 @@
+/**
+ * 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 { TreeSelect } from 'antd-v5';
+import type { TreeSelectProps } from 'antd-v5/es/tree-select';
+
+export default TreeSelect;
Review Comment:
Let's use named exports
##########
superset-frontend/src/components/Tag/Tag.test.tsx:
##########
@@ -37,7 +37,7 @@ test('should render shortname properly', () => {
const { container } = render(<Tag {...mockedProps} />);
expect(container).toBeInTheDocument();
expect(screen.getByTestId('tag')).toBeInTheDocument();
- expect(screen.getByTestId('tag')).toHaveTextContent(mockedProps.name);
+ expect(screen.getByTestId('tag')).toHaveTextContent('example-tag');
Review Comment:
Is this necessary? i think it's better to use mocked props for assertion
instead of hard coded strings.
##########
superset-frontend/src/components/Tag/utils.tsx:
##########
@@ -37,9 +37,9 @@ const cachedSupersetGet = cacheWrapper(
);
type SelectTagsValue = {
- value: number | undefined;
+ value: number | string | undefined;
label: string | undefined;
- key: number | undefined;
+ key: number | string | undefined;
Review Comment:
This seems like a functional change?
##########
superset-frontend/src/components/Upload/index.tsx:
##########
@@ -0,0 +1,23 @@
+/**
+ * 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 { Upload, type UploadFile } from 'antd-v5';
+import { type UploadChangeParam } from 'antd-v5/es/upload';
+
+export default Upload;
Review Comment:
Same here
##########
superset-frontend/src/components/Radio/Radio.test.tsx:
##########
@@ -0,0 +1,53 @@
+/**
+ * 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 { render, screen, fireEvent } from '@testing-library/react';
+import { ThemeProvider, supersetTheme } from '@superset-ui/core';
+import '@testing-library/jest-dom';
+import Button from 'src/components/Button';
+
+describe('Button Component', () => {
+ test('renders button with text', () => {
+ render(
+ <ThemeProvider theme={supersetTheme}>
+ <Button>Click Me</Button>
+ </ThemeProvider>,
+ );
+ expect(screen.getByText('Click Me')).toBeInTheDocument();
+ });
+
+ test('calls onClick handler when clicked', () => {
+ const handleClick = jest.fn();
+ render(
+ <ThemeProvider theme={supersetTheme}>
+ <Button onClick={handleClick}>Click Me</Button>
+ </ThemeProvider>,
+ );
+ fireEvent.click(screen.getByText('Click Me'));
+ expect(handleClick).toHaveBeenCalledTimes(1);
+ });
+
+ test('renders a disabled button', () => {
+ render(
+ <ThemeProvider theme={supersetTheme}>
+ <Button disabled>Disabled</Button>
+ </ThemeProvider>,
+ );
+ expect(screen.getByRole('button', { name: 'Disabled' })).toBeDisabled();
+ });
+});
Review Comment:
This test seems for a `Button` component but it's a `Radio` test?
##########
superset-frontend/src/components/Input/index.tsx:
##########
@@ -17,10 +17,13 @@
* under the License.
*/
-import { Input as AntdInput, InputNumber as AntdInputNumber } from 'antd-v5';
-
-export const Input = AntdInput;
+import { Input as Antd5Input, InputNumber as AntdInputNumber } from 'antd-v5';
export const InputNumber = AntdInputNumber;
-export const { TextArea } = AntdInput;
+export const Input = Object.assign(Antd5Input, {
Review Comment:
I think yes, if we don't make any modifications to Input assigning what's
already in it seems unneccessary. I missed this earlier.
--
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]
