-
Notifications
You must be signed in to change notification settings - Fork 16.7k
fix(SqlLab): South pane visual changes #35601
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
50e046e
d972a95
e14a28b
96e697f
f1b312f
232c427
cae826c
2f50822
df2ecf8
fc603f5
9f1542b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| /** | ||
| * 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, userEvent } from '@superset-ui/core/spec'; | ||
| import { Icons } from '@superset-ui/core/components/Icons'; | ||
| import { ActionButton } from '.'; | ||
|
|
||
| const defaultProps = { | ||
| label: 'test-action', | ||
| icon: <Icons.EditOutlined />, | ||
| onClick: jest.fn(), | ||
| }; | ||
|
|
||
| test('renders action button with icon', () => { | ||
| render(<ActionButton {...defaultProps} />); | ||
|
|
||
| const button = screen.getByRole('button'); | ||
| expect(button).toBeInTheDocument(); | ||
| expect(button).toHaveAttribute('data-test', 'test-action'); | ||
| expect(button).toHaveClass('action-button'); | ||
| }); | ||
|
|
||
| test('calls onClick when clicked', async () => { | ||
| const onClick = jest.fn(); | ||
| render(<ActionButton {...defaultProps} onClick={onClick} />); | ||
|
|
||
| const button = screen.getByRole('button'); | ||
| userEvent.click(button); | ||
|
|
||
| expect(onClick).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| test('renders with tooltip when tooltip prop is provided', async () => { | ||
| const tooltipText = 'This is a tooltip'; | ||
| render(<ActionButton {...defaultProps} tooltip={tooltipText} />); | ||
|
|
||
| const button = screen.getByRole('button'); | ||
| userEvent.hover(button); | ||
|
|
||
| const tooltip = await screen.findByRole('tooltip'); | ||
| expect(tooltip).toBeInTheDocument(); | ||
| expect(tooltip).toHaveTextContent(tooltipText); | ||
| }); | ||
|
|
||
| test('renders without tooltip when tooltip prop is not provided', async () => { | ||
| render(<ActionButton {...defaultProps} />); | ||
|
|
||
| const button = screen.getByRole('button'); | ||
| userEvent.hover(button); | ||
|
|
||
| const tooltip = screen.queryByRole('tooltip'); | ||
| expect(tooltip).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test('supports ReactElement tooltip', async () => { | ||
| const tooltipElement = <div>Custom tooltip content</div>; | ||
| render(<ActionButton {...defaultProps} tooltip={tooltipElement} />); | ||
|
|
||
| const button = screen.getByRole('button'); | ||
| userEvent.hover(button); | ||
|
|
||
| const tooltip = await screen.findByRole('tooltip'); | ||
| expect(tooltip).toBeInTheDocument(); | ||
| expect(tooltip).toHaveTextContent('Custom tooltip content'); | ||
| }); | ||
|
|
||
| test('renders different icons correctly', () => { | ||
| render(<ActionButton {...defaultProps} icon={<Icons.DeleteOutlined />} />); | ||
|
|
||
| const button = screen.getByRole('button'); | ||
| expect(button).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test('renders with custom placement for tooltip', async () => { | ||
| const tooltipText = 'Tooltip with custom placement'; | ||
| render( | ||
| <ActionButton {...defaultProps} tooltip={tooltipText} placement="bottom" />, | ||
| ); | ||
|
|
||
| const button = screen.getByRole('button'); | ||
| userEvent.hover(button); | ||
|
|
||
| const tooltip = await screen.findByRole('tooltip'); | ||
| expect(tooltip).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test('has proper accessibility attributes', () => { | ||
| render(<ActionButton {...defaultProps} />); | ||
|
|
||
| const button = screen.getByRole('button'); | ||
| expect(button).toHaveAttribute('tabIndex', '0'); | ||
| expect(button).toHaveAttribute('role', 'button'); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| /** | ||
| * 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 type { ReactElement } from 'react'; | ||
| import { | ||
| Tooltip, | ||
| type TooltipPlacement, | ||
| type IconType, | ||
| } from '@superset-ui/core/components'; | ||
| import { css, useTheme } from '@superset-ui/core'; | ||
|
|
||
| export interface ActionProps { | ||
| label: string; | ||
| tooltip?: string | ReactElement; | ||
| placement?: TooltipPlacement; | ||
| icon: IconType; | ||
| onClick: () => void; | ||
| } | ||
|
|
||
| export const ActionButton = ({ | ||
| label, | ||
| tooltip, | ||
| placement, | ||
| icon, | ||
| onClick, | ||
| }: ActionProps) => { | ||
| const theme = useTheme(); | ||
| const actionButton = ( | ||
| <span | ||
| role="button" | ||
| tabIndex={0} | ||
| css={css` | ||
| cursor: pointer; | ||
| color: ${theme.colorIcon}; | ||
| margin-right: ${theme.sizeUnit}px; | ||
| &:hover { | ||
| path { | ||
| fill: ${theme.colorPrimary}; | ||
| } | ||
| } | ||
| `} | ||
| className="action-button" | ||
| data-test={label} | ||
| onClick={onClick} | ||
| > | ||
| {icon} | ||
| </span> | ||
| ); | ||
|
|
||
| const tooltipId = `${label.replaceAll(' ', '-').toLowerCase()}-tooltip`; | ||
|
|
||
| return tooltip ? ( | ||
| <Tooltip id={tooltipId} title={tooltip} placement={placement}> | ||
| {actionButton} | ||
| </Tooltip> | ||
| ) : ( | ||
| actionButton | ||
| ); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,15 +21,18 @@ import { css, styled, useTheme } from '@superset-ui/core'; | |
| // eslint-disable-next-line no-restricted-imports | ||
| import { Tabs as AntdTabs, TabsProps as AntdTabsProps } from 'antd'; | ||
| import { Icons } from '@superset-ui/core/components/Icons'; | ||
| import type { SerializedStyles } from '@emotion/react'; | ||
|
|
||
| export interface TabsProps extends AntdTabsProps { | ||
| allowOverflow?: boolean; | ||
| contentStyle?: SerializedStyles; | ||
| } | ||
|
|
||
| const StyledTabs = ({ | ||
| animated = false, | ||
| allowOverflow = true, | ||
| tabBarStyle, | ||
| contentStyle, | ||
| ...props | ||
| }: TabsProps) => { | ||
| const theme = useTheme(); | ||
|
|
@@ -46,6 +49,7 @@ const StyledTabs = ({ | |
|
|
||
| .ant-tabs-content-holder { | ||
| overflow: ${allowOverflow ? 'visible' : 'auto'}; | ||
| ${contentStyle} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing null check for contentStyle CSS interpolation
Tell me moreWhat is the issue?The contentStyle prop is being interpolated directly into CSS without null/undefined checks, which could cause rendering issues if the prop is not provided. Why this mattersWhen contentStyle is undefined or null, the CSS interpolation will render 'undefined' or 'null' as literal text in the CSS, potentially breaking styles or causing console warnings. Suggested change ∙ Feature PreviewAdd a null check before interpolating contentStyle: ${contentStyle || ''}Or use optional chaining with nullish coalescing: ${contentStyle ?? ''}Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||
| } | ||
| .ant-tabs-tab { | ||
| flex: 1 1 auto; | ||
|
|
@@ -85,9 +89,10 @@ const Tabs = Object.assign(StyledTabs, { | |
| }); | ||
|
|
||
| const StyledEditableTabs = styled(StyledTabs)` | ||
| ${({ theme }) => ` | ||
| ${({ theme, contentStyle }) => ` | ||
| .ant-tabs-content-holder { | ||
| background: ${theme.colorBgContainer}; | ||
| ${contentStyle} | ||
| } | ||
|
|
||
| & > .ant-tabs-nav { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.