msyavuz commented on code in PR #32705: URL: https://github.com/apache/superset/pull/32705#discussion_r2005396002
########## superset-frontend/src/components/Grid/index.tsx: ########## @@ -0,0 +1,29 @@ +/** + * 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 { Row, Col } from 'antd-v5'; +import useBreakpoint from 'antd-v5/lib/grid/hooks/useBreakpoint'; +import type { ColProps, ColSize } from 'antd-v5/es/col'; +import type { RowProps } from 'antd-v5/es/row'; + +export type { ColProps, ColSize, RowProps }; + +export { Row, Col }; + +const Grid = { useBreakpoint }; Review Comment: I think we use `Object.assign` syntax in multiple places. And useBreakpoint import here seems unneccessary? We can probably assign to `Grid.useBreakpoint` ########## superset-frontend/src/components/Grid/Grid.stories.tsx: ########## @@ -0,0 +1,169 @@ +/** + * 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 { Meta, StoryObj } from '@storybook/react'; +import Slider from 'src/components/Slider/index'; +import { useState } from 'react'; +import { Row, Col } from './index'; + +export default { + title: 'Components/Grid', + component: Row, + subcomponents: { Col }, + argTypes: { + // Row properties + align: { + control: 'select', + options: ['top', 'middle', 'bottom', 'stretch'], + description: 'Vertical alignment', + }, + justify: { + control: 'select', + options: [ + 'start', + 'end', + 'center', + 'space-around', + 'space-between', + 'space-evenly', + ], + description: 'Horizontal arrangement', + }, + gutter: { + control: 'object', + description: 'Spacing between grids', + }, + wrap: { + control: 'boolean', + description: 'Auto wrap line', + }, + // Col properties + flex: { + control: 'text', + description: 'Flex layout style', + }, + offset: { + control: 'number', + description: 'The number of cells to offset Col from the left', + }, + order: { + control: 'number', + description: 'Raster order', + }, + pull: { + control: 'number', + description: 'The number of cells that raster is moved to the left', + }, + push: { + control: 'number', + description: 'The number of cells that raster is moved to the right', + }, + span: { + control: 'number', + description: 'Raster number of cells to occupy', + }, + xs: { + control: 'object', + description: 'Settings for screen < 576px', + }, + sm: { + control: 'object', + description: 'Settings for screen ≥ 576px', + }, + md: { + control: 'object', + description: 'Settings for screen ≥ 768px', + }, + lg: { + control: 'object', + description: 'Settings for screen ≥ 992px', + }, + xl: { + control: 'object', + description: 'Settings for screen ≥ 1200px', + }, + xxl: { + control: 'object', + description: 'Settings for screen ≥ 1600px', + }, + }, + parameters: { + docs: { + description: { + component: + 'Grid is a hook, but here we are testing the Row and Col components that use it.', + }, + }, + }, +} as Meta<typeof Row>; + +type Story = StoryObj<typeof Row>; + +export const GridStory: Story = { + render: () => { + const [gutter, setGutter] = useState(24); + const [vgutter, setVgutter] = useState(24); Review Comment: This seems to not have any effect on the component ########## superset-frontend/src/components/Layout/Layout.stories.tsx: ########## @@ -0,0 +1,126 @@ +/** + * 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 { useState } from 'react'; +import { Meta, StoryObj } from '@storybook/react'; +import Layout from 'src/components/Layout'; +import { Menu } from 'src/components/Menu'; +import { MenuFoldOutlined, MenuUnfoldOutlined } from '@ant-design/icons'; + +const { Header, Footer, Sider, Content } = Layout; + +export default { + title: 'Components/Layout', + component: Layout, + subcomponents: { Header, Footer, Sider, Content }, + argTypes: { + hasSider: { + control: 'boolean', + description: 'Include a sider', + }, + }, + parameters: { + docs: { + description: { + component: + 'Ant Design Layout component with configurable Sider, Header, Footer, and Content.', + }, + }, + }, +} satisfies Meta<typeof Layout>; + +type Story = StoryObj<typeof Layout>; + +export const LayoutStory: Story = { + args: { + hasSider: true, + }, + render: args => { + const theme: 'light' | 'dark' = 'dark'; + const collapsible = true; + const collapsedWidth = 80; + const width = 200; + const reverseArrow = false; + const breakpoint: 'xs' | 'sm' | 'md' | 'lg' | 'xl' | 'xxl' = 'md'; + const headerVisible = true; + const footerVisible = true; + const [collapsed, setCollapsed] = useState(false); // Solo questa rimane con useState Review Comment: Instead of this state we can use the args here as well to make it interactive. Also this story can only control hasSider ########## superset-frontend/src/components/Layout/Layout.test.tsx: ########## @@ -0,0 +1,112 @@ +/** + * 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 { useState, ReactElement } from 'react'; +import { render, screen, fireEvent } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import Layout from 'src/components/Layout'; +import Button from 'src/components/Button'; +import { ThemeProvider, supersetTheme } from '@superset-ui/core'; + +describe('Layout Component', () => { + const renderWithTheme = (ui: ReactElement) => + render(<ThemeProvider theme={supersetTheme}>{ui}</ThemeProvider>); Review Comment: We generally don't use ThemeProvider in tests and test with default ant design classes without prefix ########## superset-frontend/src/components/Layout/Layout.stories.tsx: ########## @@ -0,0 +1,126 @@ +/** + * 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 { useState } from 'react'; +import { Meta, StoryObj } from '@storybook/react'; +import Layout from 'src/components/Layout'; +import { Menu } from 'src/components/Menu'; +import { MenuFoldOutlined, MenuUnfoldOutlined } from '@ant-design/icons'; + +const { Header, Footer, Sider, Content } = Layout; + +export default { + title: 'Components/Layout', + component: Layout, + subcomponents: { Header, Footer, Sider, Content }, + argTypes: { + hasSider: { + control: 'boolean', + description: 'Include a sider', + }, + }, + parameters: { + docs: { + description: { + component: + 'Ant Design Layout component with configurable Sider, Header, Footer, and Content.', + }, + }, + }, +} satisfies Meta<typeof Layout>; + +type Story = StoryObj<typeof Layout>; + +export const LayoutStory: Story = { + args: { + hasSider: true, + }, + render: args => { + const theme: 'light' | 'dark' = 'dark'; + const collapsible = true; + const collapsedWidth = 80; + const width = 200; + const reverseArrow = false; + const breakpoint: 'xs' | 'sm' | 'md' | 'lg' | 'xl' | 'xxl' = 'md'; + const headerVisible = true; + const footerVisible = true; + const [collapsed, setCollapsed] = useState(false); // Solo questa rimane con useState + + return ( + <Layout style={{ minHeight: '100vh' }}> + {args.hasSider && ( + <Sider + collapsible={collapsible} + collapsed={collapsed} + onCollapse={setCollapsed} + theme={theme} + width={width} + collapsedWidth={collapsedWidth} + reverseArrow={reverseArrow} + breakpoint={breakpoint} + > + <div + className="logo" + style={{ + height: '32px', + margin: '16px', + background: '#ffffff30', + }} + /> + <Menu defaultSelectedKeys={['1']} mode="inline"> + <Menu.Item key="1" icon={<MenuUnfoldOutlined />}> Review Comment: Let's use the `options` prop on menu instead ########## superset-frontend/src/components/Layout/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 Layout from 'antd-v5/lib/layout'; Review Comment: This should probably be `es` as well. And now that i think about it maybe this pr should also be based off of theming branch? ########## 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'; Review Comment: Same `lib` problem 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: There are other examples of default exports but i think it's better to use named exports ########## 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'; Review Comment: Instead of importing from lib we should get it from Tree object. ########## superset-frontend/src/components/AutoComplete/AutoComplete.stories.tsx: ########## @@ -0,0 +1,208 @@ +/** + * 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 { useState } from 'react'; +import { Meta, StoryObj } from '@storybook/react'; +import AutoComplete, { + AntAutoCompleteProps, +} from 'src/components/AutoComplete'; + +export default { + title: 'Components/AutoComplete', + component: AutoComplete, + argTypes: { + style: { + control: 'object', + description: 'Custom styles for AutoComplete', + }, + placeholder: { + control: 'text', + description: 'Placeholder text for AutoComplete', + }, + disabled: { + control: 'boolean', + description: 'Disable the AutoComplete', + defaultValue: false, + }, + popupMatchSelectWidth: { + control: 'number', + description: 'Width of the dropdown', + defaultValue: 252, + }, + children: { + control: 'text', + description: 'Custom input inside AutoComplete', + }, + allowClear: { + control: 'boolean', + description: 'Show clear button', + defaultValue: false, + }, + autoFocus: { + control: 'boolean', + description: 'If get focus when component mounted', + defaultValue: false, + }, + backfill: { + control: 'boolean', + description: 'If backfill selected item the input when using keyboard', + defaultValue: false, + }, + childrenInput: { + control: 'text', + description: 'Customize input element', + }, + defaultActiveFirstOption: { + control: 'boolean', + description: 'Whether active first option by default', + defaultValue: true, + }, + defaultOpen: { + control: 'boolean', + description: 'Initial open state of dropdown', + }, + defaultValue: { + control: 'text', + description: 'Initial selected option', + }, + dropdownRender: { + control: false, + description: 'Customize dropdown content', + }, + popupClassName: { + control: 'text', + description: 'The className of dropdown menu', + }, + filterOption: { + control: 'boolean', + description: 'If true, filter options by input', + defaultValue: true, + }, + getPopupContainer: { + control: false, + description: 'Parent node of the dropdown.', + defaultValue: () => document.body, + }, + notFoundContent: { + control: 'text', + description: 'Specify content to show when no result matches', + }, + open: { + control: 'boolean', + description: 'Controlled open state of dropdown', + }, + options: { + control: 'object', + description: 'Select options. Will get better perf than JSX definition', + }, + status: { + control: 'select', + options: ['error', 'warning'], + description: 'Set validation status', + }, + size: { + control: 'select', + options: ['large', 'middle', 'small'], + description: 'The size of the input box', + }, + value: { + control: 'text', + description: 'Selected option', + }, + variant: { + control: 'select', + options: ['outlined', 'borderless', 'filled'], + description: 'Variants of input', + defaultValue: 'outlined', + }, + virtual: { + control: 'boolean', + description: 'Disable virtual scroll when set to false', + defaultValue: true, + }, + }, + parameters: { + docs: { + description: { + component: 'AutoComplete component for search functionality.', + }, + }, + }, +} as Meta<typeof AutoComplete>; + +const getRandomInt = (max: number, min = 0) => + Math.floor(Math.random() * (max - min + 1)) + min; + +const searchResult = (query: string) => + Array.from({ length: getRandomInt(5) }).map((_, idx) => { + const category = `${query}${idx}`; + return { + value: category, + label: ( + <div style={{ display: 'flex', justifyContent: 'space-between' }}> + <span> + Found {query} on{' '} + <a + href={`https://github.com/apache/superset?q=${query}`} + target="_blank" + rel="noopener noreferrer" + > + {category} + </a> + </span> + <span>{getRandomInt(200, 100)} results</span> + </div> + ), + }; + }); + +const AutoCompleteWithOptions = (args: AntAutoCompleteProps) => { + const [options, setOptions] = useState<AntAutoCompleteProps['options']>([]); + + const handleSearch = (value: string) => { + setOptions(value ? searchResult(value) : []); + }; + + return <AutoComplete {...args} options={options} onSearch={handleSearch} />; +}; +type Story = StoryObj<typeof AutoComplete>; + +export const AutoCompleteStory: Story = { + args: { + style: { width: 300 }, + placeholder: 'Type to search...', + disabled: false, + allowClear: true, + autoFocus: false, + backfill: false, + defaultActiveFirstOption: true, + defaultOpen: false, + defaultValue: '', + filterOption: true, + notFoundContent: 'No results found', + open: false, + size: 'middle', + variant: 'outlined', + virtual: true, + }, + render: (args: AntAutoCompleteProps) => ( + <div style={{ margin: '20px' }}> + <AutoCompleteWithOptions {...args} /> + </div> + ), +}; Review Comment: I don't think this story is functional ########## superset-frontend/src/components/Grid/Grid.test.tsx: ########## @@ -0,0 +1,36 @@ +/** + * 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 } from '@testing-library/react'; +import '@testing-library/jest-dom'; Review Comment: is this neccessary? ########## superset-frontend/src/components/AutoComplete/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 AutoComplete, { + AutoCompleteProps as AntAutoCompleteProps, +} from 'antd-v5/lib/auto-complete'; + +export type { AntAutoCompleteProps }; Review Comment: This renaming seems unneccessary ########## 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, { + TextArea: Antd5Input.TextArea, + Search: Antd5Input.Search, + Password: Antd5Input.Password, + OTP: Antd5Input.OTP, +}); Review Comment: This seems better! -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
