bito-code-review[bot] commented on code in PR #37681:
URL: https://github.com/apache/superset/pull/37681#discussion_r2885445475
##########
superset-frontend/packages/superset-ui-core/src/components/Tabs/Tabs.tsx:
##########
@@ -17,81 +17,88 @@
* under the License.
*/
import type { FC } from 'react';
-import { css, styled, useTheme } from '@apache-superset/core/ui';
-
-// eslint-disable-next-line no-restricted-imports
+import { css, styled } from '@apache-superset/core/ui';
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;
+ contentHeight?: string | number;
fullHeight?: boolean;
contentStyle?: SerializedStyles;
+ contentPadding?: SerializedStyles;
}
const StyledTabs = ({
animated = false,
allowOverflow = true,
+ contentHeight = '100%',
fullHeight = false,
tabBarStyle,
contentStyle,
+ contentPadding,
...props
-}: TabsProps) => {
- const theme = useTheme();
- const defaultTabBarStyle = { paddingLeft: theme.sizeUnit * 4 };
- const mergedStyle = { ...defaultTabBarStyle, ...tabBarStyle };
-
- return (
- <AntdTabs
- animated={animated}
- {...props}
- tabBarStyle={mergedStyle}
- css={theme => css`
- overflow: ${allowOverflow ? 'visible' : 'hidden'};
+}: TabsProps) => (
+ <AntdTabs
+ animated={animated}
+ {...props}
+ tabBarStyle={tabBarStyle}
+ css={theme => css`
+ overflow: ${allowOverflow ? 'visible' : 'hidden'};
+ ${fullHeight && 'height: 100%;'}
+
+ .ant-tabs-content-holder {
+ overflow: ${allowOverflow ? 'visible' : 'auto'};
${fullHeight && 'height: 100%;'}
-
- .ant-tabs-content-holder {
- overflow: ${allowOverflow ? 'visible' : 'auto'};
- ${fullHeight && 'height: 100%;'}
- }
- .ant-tabs-content {
- ${fullHeight && 'height: 100%;'}
- }
- .ant-tabs-tabpane {
- ${fullHeight && 'height: 100%;'}
- ${contentStyle}
- }
- .ant-tabs-tab {
- flex: 1 1 auto;
-
- .short-link-trigger.btn {
- padding: 0 ${theme.sizeUnit}px;
- & > .fa.fa-link {
- top: 0;
- }
+ ${contentHeight &&
+ `height: ${typeof contentHeight === 'number' ? `${contentHeight}px` :
contentHeight};`}
+ ${contentPadding}
+ }
+ .ant-tabs-content {
+ ${fullHeight && 'height: 100%;'}
+ }
+ .ant-tabs-tabpane {
+ ${fullHeight && 'height: 100%;'}
+ ${contentStyle}
+ }
+ .ant-tabs-nav {
+ margin: 0;
+ }
+ .ant-tabs-nav-wrap {
+ ${!(tabBarStyle && 'paddingLeft' in tabBarStyle)
+ ? `padding: 0 ${theme.sizeUnit * 4}px;`
+ : ''}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Padding selector change breaks tests</b></div>
<div id="fix">
The padding logic was refactored from merging into tabBarStyle (applied to
.ant-tabs-nav) to conditional CSS on .ant-tabs-nav-wrap. However, existing
tests in Tabs.test.tsx expect paddingLeft on .ant-tabs-nav, which will now
fail. This changes the visual padding location and breaks test
assertions—consider if this selector change is intentional or revert to
maintain compatibility.
</div>
</div>
<small><i>Code Review Run #437ce3</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/packages/superset-ui-core/src/components/Tabs/Tabs.tsx:
##########
@@ -17,81 +17,88 @@
* under the License.
*/
import type { FC } from 'react';
-import { css, styled, useTheme } from '@apache-superset/core/ui';
-
-// eslint-disable-next-line no-restricted-imports
+import { css, styled } from '@apache-superset/core/ui';
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;
+ contentHeight?: string | number;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Default contentHeight changes layout</b></div>
<div id="fix">
Adding contentHeight with default '100%' now sets height: 100% on
.ant-tabs-content-holder by default, changing the component's default layout
behavior. Previously, no height was applied unless fullHeight was true. This
could affect existing usages expecting no default height—consider making the
default undefined to preserve backward compatibility.
</div>
</div>
<small><i>Code Review Run #437ce3</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
