alexandrusoare commented on code in PR #31972:
URL: https://github.com/apache/superset/pull/31972#discussion_r1935539319
##########
superset-frontend/src/features/charts/ChartCard.tsx:
##########
@@ -172,9 +172,9 @@ export default function ChartCard({
isStarred={favoriteStatus}
/>
)}
- <AntdDropdown overlay={menu}>
+ <Dropdown dropdownRender={() => menu}>
<Icons.MoreVert iconColor={theme.colors.grayscale.base} />
Review Comment:
Maybe some need the `dropdownRender` and some the `menu` props
##########
superset-frontend/cypress-base/cypress/e2e/explore/chart.test.js:
##########
@@ -99,7 +99,7 @@ describe('Cross-referenced dashboards', () => {
cy.wait('@filtering');
});
- it('should show the cross-referenced dashboards', () => {
+ it.only('should show the cross-referenced dashboards', () => {
visitSampleChartFromList('1 - Sample chart');
Review Comment:
Ditto
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.jsx:
##########
@@ -109,7 +109,7 @@ export const MenuTrigger = styled(Button)`
`;
const iconReset = css`
- .ant-dropdown-menu-item > & > .anticon:first-child {
+ .antd5-dropdown-menu-item > & > .anticon:first-child {
margin-right: 0;
vertical-align: 0;
}
Review Comment:
Here too?
##########
superset-frontend/src/SqlLab/components/QueryLimitSelect/index.tsx:
##########
@@ -95,8 +95,8 @@ const QueryLimitSelect = ({
return (
<LimitSelectStyled>
- <AntdDropdown
- overlay={renderQueryLimit(maxRow, setQueryLimit)}
+ <Dropdown
+ dropdownRender={() => renderQueryLimit(maxRow, setQueryLimit)}
trigger={['click']}
Review Comment:
I checked the situation with `overlay` and AntDesign suggests using `menu`
props instead. Why chose `dropdownRender` ?
##########
superset-frontend/src/SqlLab/components/QueryLimitSelect/index.tsx:
##########
@@ -36,7 +36,7 @@ export function convertToNumWithSpaces(num: number) {
const LimitSelectStyled = styled.span`
${({ theme }) => `
- .ant-dropdown-trigger {
+ .antd5-dropdown-trigger {
align-items: center;
color: ${theme.colors.grayscale.dark2};
display: flex;
Review Comment:
Here as well, is the style still needed, especially the `color` ?
##########
superset-frontend/src/components/index.ts:
##########
@@ -56,7 +56,6 @@ export {
Card as AntdCard,
Checkbox as AntdCheckbox,
Collapse as AntdCollapse,
- Dropdown as AntdDropdown,
Form as AntdForm,
Review Comment:
Also the export of `DropdownProps` should be removed from these files
##########
superset-frontend/cypress-base/cypress/e2e/dashboard/horizontalFilterBar.test.ts:
##########
@@ -57,24 +57,24 @@ function setFilterBarOrientation(orientation: 'vertical' |
'horizontal') {
.trigger('mouseover');
if (orientation === 'vertical') {
- cy.get('.antd5-menu-item-selected')
+ cy.get('.antd5-dropdown-menu-item-selected')
.contains('Horizontal (Top)')
.should('exist');
- cy.get('.antd5-menu-item').contains('Vertical (Left)').click();
+ cy.get('.antd5-dropdown-menu-item').contains('Vertical (Left)').click();
cy.getBySel('dashboard-filters-panel').should('exist');
} else {
- cy.get('.antd5-menu-item-selected')
+ cy.get('.antd5-dropdown-menu-item-selected')
.contains('Vertical (Left)')
.should('exist');
- cy.get('.antd5-menu-item').contains('Horizontal (Top)').click();
+ cy.get('.antd5-dropdown-menu-item').contains('Horizontal (Top)').click();
cy.getBySel('loading-indicator').should('exist');
cy.getBySel('filter-bar').should('exist');
cy.getBySel('dashboard-filters-panel').should('not.exist');
}
}
describe('Horizontal FilterBar', () => {
- it('should go from vertical to horizontal and the opposite', () => {
+ it.only('should go from vertical to horizontal and the opposite', () => {
visitDashboard();
Review Comment:
Forgot an `only` 😄
##########
superset-frontend/src/GlobalStyles.tsx:
##########
@@ -104,11 +103,11 @@ export const GlobalStyles = () => (
margin-right: 0;
}
}
- .ant-dropdown-menu-sub .antd5-menu.antd5-menu-vertical {
+ .antd5-dropdown-menu-sub .antd5-menu.antd5-menu-vertical {
box-shadow: none;
}
- .ant-dropdown-menu-submenu-title,
- .ant-dropdown-menu-item {
+ .antd5-dropdown-menu-submenu-title,
+ .antd5-dropdown-menu-item {
line-height: 1.5em !important;
}
Review Comment:
Are theses styles still important?
##########
superset-frontend/src/GlobalStyles.tsx:
##########
@@ -44,7 +44,6 @@ export const GlobalStyles = () => (
// Prefer vanilla Ant Design z-indexes that should work out of the box
.ant-popover,
.antd5-dropdown,
- .ant-dropdown,
Review Comment:
After upgrading the `Popover` component, its `z-index` was set by default to
`3030` so I didn't need to declare it here anymore, can you check that the
global style is still needed for `Dropdown` ?
##########
superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx:
##########
@@ -82,7 +82,7 @@ const Styles = styled.div`
.error-alert {
margin: ${({ theme }) => 2 * theme.gridUnit}px;
}
- .ant-dropdown-trigger {
+ .antd5-dropdown-trigger {
margin-left: ${({ theme }) => 2 * theme.gridUnit}px;
box-shadow: none;
&:active {
Review Comment:
here as well, is the style needed?
##########
superset-frontend/src/components/Menu/index.tsx:
##########
@@ -81,7 +81,7 @@ const StyledMenu = styled(AntdMenu)`
border-bottom: 1px solid transparent;
}
&.antd5-menu-vertical,
- &.ant-dropdown-menu {
+ &.antd5-dropdown-menu {
box-shadow:
0 3px 6px -4px ${addAlpha(theme.colors.grayscale.dark2, 0.12)},
0 6px 16px 0
Review Comment:
Same here, are the styles still needed?
##########
superset-frontend/src/dashboard/hooks/useDownloadScreenshot.ts:
##########
@@ -0,0 +1,184 @@
+/**
+ * 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 { useCallback, useEffect, useRef } from 'react';
+import { useSelector } from 'react-redux';
+import { useToasts } from 'src/components/MessageToasts/withToasts';
+import { last } from 'lodash';
+import {
+ logging,
+ t,
+ SupersetClient,
+ SupersetApiError,
+} from '@superset-ui/core';
+import {
+ LOG_ACTIONS_DASHBOARD_DOWNLOAD_AS_IMAGE,
+ LOG_ACTIONS_DASHBOARD_DOWNLOAD_AS_PDF,
+} from 'src/logger/LogUtils';
+import { RootState } from 'src/dashboard/types';
+import { getDashboardUrlParams } from 'src/utils/urlUtils';
+import { DownloadScreenshotFormat } from
'../components/menu/DownloadMenuItems/types';
+
+const RETRY_INTERVAL = 3000;
+const MAX_RETRIES = 30;
+
+export const useDownloadScreenshot = (
+ dashboardId: number,
+ logEvent?: Function,
+) => {
+ const activeTabs = useSelector(
+ (state: RootState) => state.dashboardState.activeTabs || undefined,
+ );
+ const anchor = useSelector(
+ (state: RootState) =>
+ last(state.dashboardState.directPathToChild) || undefined,
+ );
+ const dataMask = useSelector(
+ (state: RootState) => state.dataMask || undefined,
+ );
+
+ const { addDangerToast, addSuccessToast, addInfoToast } = useToasts();
+
+ const currentIntervalIds = useRef<NodeJS.Timeout[]>([]);
+
+ const stopIntervals = useCallback(
+ (message?: 'success' | 'failure') => {
+ currentIntervalIds.current.forEach(clearInterval);
+
+ if (message === 'failure') {
+ addDangerToast(
+ t('The screenshot could not be downloaded. Please, try again
later.'),
+ );
+ }
+ if (message === 'success') {
+ addSuccessToast(t('The screenshot has been downloaded.'));
+ }
+ },
+ [addDangerToast, addSuccessToast],
+ );
+
Review Comment:
What's the utility of this new file, and does it fall under the `dropdown`
upgrade?
--
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
