-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
fix: fix adhocpopovers tab animate. #14478
Conversation
Could we disable the animation globally by default? E.g. like this: diff --git a/superset-frontend/src/components/Tabs/Tabs.tsx b/superset-frontend/src/components/Tabs/Tabs.tsx
index 90c3a64e6..f016b73f3 100644
--- a/superset-frontend/src/components/Tabs/Tabs.tsx
+++ b/superset-frontend/src/components/Tabs/Tabs.tsx
@@ -20,81 +20,92 @@ import React from 'react';
import { css, styled } from '@superset-ui/core';
import AntDTabs, { TabsProps as AntDTabsProps } from 'antd/lib/tabs';
import Icon from 'src/components/Icon';
+import { useTheme } from '@emotion/react';
export interface TabsProps extends AntDTabsProps {
fullWidth?: boolean;
allowOverflow?: boolean;
}
-const notForwardedProps = ['fullWidth', 'allowOverflow'];
+const StyledTabs = ({
+ animated = false,
+ fullWidth = true,
+ allowOverflow = true,
+ ...props
+}: TabsProps) => {
+ const theme = useTheme();
+ return (
+ <AntDTabs
+ animated={animated}
+ {...props}
+ css={css`
+ overflow: ${allowOverflow ? 'visible' : 'hidden'};
+
+ .ant-tabs-content-holder {
+ overflow: ${allowOverflow ? 'visible' : 'auto'};
+ }
-const StyledTabs = styled(AntDTabs, {
- shouldForwardProp: prop => !notForwardedProps.includes(String(prop)),
-})<TabsProps>`
- overflow: ${({ allowOverflow }) => (allowOverflow ? 'visible' : 'hidden')};
+ .ant-tabs-tab {
+ flex: 1 1 auto;
- .ant-tabs-content-holder {
- overflow: ${({ allowOverflow }) => (allowOverflow ? 'visible' : 'auto')};
- }
+ &.ant-tabs-tab-active .ant-tabs-tab-btn {
+ color: inherit;
+ }
- .ant-tabs-tab {
- flex: 1 1 auto;
+ &:hover {
+ .anchor-link-container {
+ cursor: pointer;
- &.ant-tabs-tab-active .ant-tabs-tab-btn {
- color: inherit;
- }
+ .fa.fa-link {
+ visibility: visible;
+ }
+ }
+ }
- &:hover {
- .anchor-link-container {
- cursor: pointer;
+ .short-link-trigger.btn {
+ padding: 0 ${theme.gridUnit}px;
- .fa.fa-link {
- visibility: visible;
+ & > .fa.fa-link {
+ top: 0;
+ }
+ }
}
- }
- }
-
- .short-link-trigger.btn {
- padding: 0 ${({ theme }) => theme.gridUnit}px;
-
- & > .fa.fa-link {
- top: 0;
- }
- }
- }
-
- ${({ fullWidth }) =>
- fullWidth &&
- css`
- .ant-tabs-nav-list {
- width: 100%;
- }
-
- .ant-tabs-tab {
- width: 0;
- }
- `};
- .ant-tabs-tab-btn {
- display: flex;
- flex: 1 1 auto;
- align-items: center;
- justify-content: center;
- font-size: ${({ theme }) => theme.typography.sizes.s}px;
- text-align: center;
- text-transform: uppercase;
- user-select: none;
-
- .required {
- margin-left: ${({ theme }) => theme.gridUnit / 2}px;
- color: ${({ theme }) => theme.colors.error.base};
- }
- }
+ ${fullWidth &&
+ css`
+ .ant-tabs-nav-list {
+ width: 100%;
+ }
+
+ .ant-tabs-tab {
+ width: 0;
+ margin-right: 0;
+ }
+ `};
+
+ .ant-tabs-tab-btn {
+ display: flex;
+ flex: 1 1 auto;
+ align-items: center;
+ justify-content: center;
+ font-size: ${theme.typography.sizes.s}px;
+ text-align: center;
+ text-transform: uppercase;
+ user-select: none;
+
+ .required {
+ margin-left: ${theme.gridUnit / 2}px;
+ color: ${theme.colors.error.base};
+ }
+ }
- .ant-tabs-ink-bar {
- background: ${({ theme }) => theme.colors.secondary.base};
- }
-`;
+ .ant-tabs-ink-bar {
+ background: ${theme.colors.secondary.base};
+ }
+ `}
+ />
+ );
+};
const StyledTabPane = styled(AntDTabs.TabPane)``;
@@ -102,11 +113,6 @@ const Tabs = Object.assign(StyledTabs, {
TabPane: StyledTabPane,
});
-Tabs.defaultProps = {
- fullWidth: true,
- animated: true,
-};
-
const StyledEditableTabs = styled(StyledTabs)`
.ant-tabs-content-holder {
background: white;
Note this diff also fixes a bug where tabs are not expanded to full width: BeforeAfter |
Codecov Report
@@ Coverage Diff @@
## master #14478 +/- ##
==========================================
- Coverage 77.47% 77.47% -0.01%
==========================================
Files 958 958
Lines 48486 48483 -3
Branches 5679 5683 +4
==========================================
- Hits 37565 37562 -3
Misses 10721 10721
Partials 200 200
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
what's the good case for keeping animation on as default? Note this change would not prevent us from turning on the animation when needed. And if we do consciously decide we want animation in the future, it's literally just a flip of a switch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! with minor suggestions.
...rset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopover/index.jsx
Outdated
Show resolved
Hide resolved
...rset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx
Outdated
Show resolved
Hide resolved
/testenv up |
@rusackas Ephemeral environment spinning up at http://35.165.160.43:8080. Credentials are |
superset-frontend/spec/javascripts/dashboard/components/DashboardBuilder_spec.jsx
Outdated
Show resolved
Hide resolved
@pkdotson please rebase, I think this is otherwise good to go. |
Ephemeral environment shutdown and build artifacts deleted. |
* fix popover * addd tabs default css * fix lint * fix tests * address comments * lint fix * fix test * lint
* fix popover * addd tabs default css * fix lint * fix tests * address comments * lint fix * fix test * lint
* fix popover * addd tabs default css * fix lint * fix tests * address comments * lint fix * fix test * lint
SUMMARY
The tab component within the adhocFilterEditpopvers were showing an animated view within the tab when user clicks on new tabs. This pr removes the default animate to false
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
before
https://user-images.githubusercontent.com/17326228/117086414-4a6a8700-ad01-11eb-9a1c-6f29b20c2d9c.mov
after
Screen.Recording.2021-05-04.at.5.43.01.PM.mov
TEST PLAN
Go to chart and open the popovers to ensure they do not animate when new tab is clicked.
ADDITIONAL INFORMATION