You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "msyavuz (via GitHub)" <gi...@apache.org> on 2025/03/11 08:55:31 UTC
[PR] refactor(Menu): Move to usage of items prop instead of Menu.Item since Menu.Item is deprecated [superset]
msyavuz opened a new pull request, #32587:
URL: https://github.com/apache/superset/pull/32587
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Ant Design 5 marks Menu.Item as deprecated. This PR aims to move away from that to items prop as advised.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
Run the testing suite.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Menu): Move to usage of items prop instead of Menu.Item since Menu.Item is deprecated [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #32587:
URL: https://github.com/apache/superset/pull/32587#discussion_r2148818131
##########
superset-frontend/src/components/GridTable/HeaderMenu.tsx:
##########
@@ -41,129 +41,199 @@ type Params = {
onVisibleChange: DropdownProps['onOpenChange'];
};
-const HeaderMenu: React.FC<Params> = ({
+const HeaderMenu: React.FC<HeaderMenuProps> = ({
colId,
api,
pinnedLeft,
pinnedRight,
invisibleColumns,
isMain,
onVisibleChange,
-}: Params) => {
+}: HeaderMenuProps) => {
const pinColumn = useCallback(
(pinLoc: ColumnPinnedType) => {
api.setColumnsPinned([colId], pinLoc);
},
[api, colId],
);
- const unHideAction = invisibleColumns.length > 0 && (
- <Menu.SubMenu title={t('Unhide')} icon={<Icons.EyeOutlined iconSize="m" />}>
- {invisibleColumns.length > 1 && (
- <Menu.Item
- onClick={() => {
- api.setColumnsVisible(invisibleColumns, true);
- }}
- >
- <b>{t('All %s hidden columns', invisibleColumns.length)}</b>
- </Menu.Item>
- )}
- {invisibleColumns.map(c => (
- <Menu.Item
- key={c.getColId()}
- onClick={() => {
- api.setColumnsVisible([c.getColId()], true);
- }}
- >
- {c.getColDef().headerName}
- </Menu.Item>
- ))}
- </Menu.SubMenu>
- );
+ const unHideAction: MenuItem = {
+ label: t('Unhide'),
+ key: 'unHideSubMenu',
+ icon: <Icons.EyeInvisibleOutlined iconSize="m" />,
+ children: [
+ invisibleColumns.length > 1 && {
+ key: 'allHidden',
+ label: <b>{t('All %s hidden columns', invisibleColumns.length)}</b>,
+ onClick: () => {
+ api.setColumnsVisible(invisibleColumns, true);
+ },
+ },
+ ...invisibleColumns.map(c => ({
+ key: c.getColId(),
+ label: c.getColDef().headerName,
+ onClick: () => {
+ api.setColumnsVisible([c.getColId()], true);
+ },
+ })),
+ ].filter(Boolean) as MenuItem[],
+ };
+
+ const mainMenuItems: MenuItem[] = [
+ {
+ key: 'copyData',
+ label: t('Copy the current data'),
+ icon: <Icons.CopyOutlined iconSize="m" />,
+ onClick: () => {
+ copyTextToClipboard(
+ () =>
+ new Promise((resolve, reject) => {
+ const data = api.getDataAsCsv({
+ columnKeys: api
+ .getAllDisplayedColumns()
+ .map(c => c.getColId())
+ .filter(id => id !== colId),
+ suppressQuotes: true,
+ columnSeparator: '\t',
+ });
+ if (data) {
+ resolve(data);
+ } else {
+ reject();
+ }
Review Comment:
### Empty Promise Rejection <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Empty reject call in Promise without providing error information.
###### Why this matters
When the Promise is rejected, no error information is passed, making it impossible to understand why the operation failed during debugging.
###### Suggested change β *Feature Preview*
```typescript
if (data) {
resolve(data);
} else {
reject(new Error('Failed to get CSV data'));
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e01df21e-8495-41c4-a439-9eb07ad7bb29/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e01df21e-8495-41c4-a439-9eb07ad7bb29?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e01df21e-8495-41c4-a439-9eb07ad7bb29?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e01df21e-8495-41c4-a439-9eb07ad7bb29?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e01df21e-8495-41c4-a439-9eb07ad7bb29)
</details>
<sub>
π¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:29bde402-d275-4fdd-a94e-20c6e8fcf393 -->
[](29bde402-d275-4fdd-a94e-20c6e8fcf393)
##########
superset-frontend/src/SqlLab/components/SaveQuery/index.tsx:
##########
@@ -111,16 +111,18 @@ const SaveQuery = ({
database?.allows_virtual_table_explore !== undefined;
const overlayMenu = (
- <Menu>
- <Menu.Item
- onClick={() => {
- logAction(LOG_ACTIONS_SQLLAB_CREATE_CHART, {});
- setShowSaveDatasetModal(true);
- }}
- >
- {t('Save dataset')}
- </Menu.Item>
- </Menu>
+ <Menu
+ items={[
+ {
+ label: t('Save dataset'),
+ key: 'save-dataset',
+ onClick: () => {
+ logAction(LOG_ACTIONS_SQLLAB_CREATE_CHART, {});
+ setShowSaveDatasetModal(true);
+ },
+ },
+ ]}
+ />
Review Comment:
### Misleading Action Logging <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The logAction is logging 'CREATE_CHART' when saving a dataset, which is misleading and doesn't match the actual action being performed.
###### Why this matters
Inconsistent documentation through logging makes it harder to track and debug user actions accurately in the system.
###### Suggested change β *Feature Preview*
Change the log action to be more descriptive of the actual operation:
```typescript
logAction(LOG_ACTIONS_SQLLAB_SAVE_DATASET, {});
```
And ensure LOG_ACTIONS_SQLLAB_SAVE_DATASET is added to the imports from LogUtils.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bf180797-6c9b-47a9-8e12-0f1cfc91bccb/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bf180797-6c9b-47a9-8e12-0f1cfc91bccb?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bf180797-6c9b-47a9-8e12-0f1cfc91bccb?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bf180797-6c9b-47a9-8e12-0f1cfc91bccb?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bf180797-6c9b-47a9-8e12-0f1cfc91bccb)
</details>
<sub>
π¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:be421efe-2f21-4f12-83e5-e97553f99927 -->
[](be421efe-2f21-4f12-83e5-e97553f99927)
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Menu): Move to usage of items prop instead of Menu.Item since Menu.Item is deprecated [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on PR #32587:
URL: https://github.com/apache/superset/pull/32587#issuecomment-2728617930
There is [this](https://github.com/ant-design/ant-design/issues/53183) issue that highlights the problem with `items` prop.
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Menu): Move to usage of items prop instead of Menu.Item since Menu.Item is deprecated [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #32587:
URL: https://github.com/apache/superset/pull/32587#discussion_r2149789765
##########
superset-frontend/src/components/Dropdown/index.tsx:
##########
@@ -82,7 +82,7 @@ export enum IconOrientation {
}
export interface MenuDotsDropdownProps extends AntdDropdownProps {
- overlay: ReactElement;
+ overlay?: ReactElement;
Review Comment:
Just wondering, is this prop necessary here?
##########
superset-frontend/src/components/GridTable/HeaderMenu.tsx:
##########
@@ -41,129 +41,199 @@ type Params = {
onVisibleChange: DropdownProps['onOpenChange'];
};
-const HeaderMenu: React.FC<Params> = ({
+const HeaderMenu: React.FC<HeaderMenuProps> = ({
colId,
api,
pinnedLeft,
pinnedRight,
invisibleColumns,
isMain,
onVisibleChange,
-}: Params) => {
+}: HeaderMenuProps) => {
const pinColumn = useCallback(
(pinLoc: ColumnPinnedType) => {
api.setColumnsPinned([colId], pinLoc);
},
[api, colId],
);
- const unHideAction = invisibleColumns.length > 0 && (
- <Menu.SubMenu title={t('Unhide')} icon={<Icons.EyeOutlined iconSize="m" />}>
- {invisibleColumns.length > 1 && (
- <Menu.Item
- onClick={() => {
- api.setColumnsVisible(invisibleColumns, true);
- }}
- >
- <b>{t('All %s hidden columns', invisibleColumns.length)}</b>
- </Menu.Item>
- )}
- {invisibleColumns.map(c => (
- <Menu.Item
- key={c.getColId()}
- onClick={() => {
- api.setColumnsVisible([c.getColId()], true);
- }}
- >
- {c.getColDef().headerName}
- </Menu.Item>
- ))}
- </Menu.SubMenu>
- );
+ const unHideAction: MenuItem = {
+ label: t('Unhide'),
+ key: 'unHideSubMenu',
+ icon: <Icons.EyeInvisibleOutlined iconSize="m" />,
+ children: [
+ invisibleColumns.length > 1 && {
+ key: 'allHidden',
+ label: <b>{t('All %s hidden columns', invisibleColumns.length)}</b>,
+ onClick: () => {
+ api.setColumnsVisible(invisibleColumns, true);
+ },
+ },
+ ...invisibleColumns.map(c => ({
+ key: c.getColId(),
+ label: c.getColDef().headerName,
+ onClick: () => {
+ api.setColumnsVisible([c.getColId()], true);
+ },
+ })),
+ ].filter(Boolean) as MenuItem[],
+ };
+
+ const mainMenuItems: MenuItem[] = [
+ {
+ key: 'copyData',
+ label: t('Copy the current data'),
+ icon: <Icons.CopyOutlined iconSize="m" />,
+ onClick: () => {
+ copyTextToClipboard(
+ () =>
+ new Promise((resolve, reject) => {
+ const data = api.getDataAsCsv({
+ columnKeys: api
+ .getAllDisplayedColumns()
+ .map(c => c.getColId())
+ .filter(id => id !== colId),
+ suppressQuotes: true,
+ columnSeparator: '\t',
+ });
+ if (data) {
+ resolve(data);
+ } else {
+ reject();
Review Comment:
Should we add an error message?
##########
superset-frontend/src/components/GridTable/HeaderMenu.tsx:
##########
@@ -41,129 +41,199 @@ type Params = {
onVisibleChange: DropdownProps['onOpenChange'];
};
-const HeaderMenu: React.FC<Params> = ({
+const HeaderMenu: React.FC<HeaderMenuProps> = ({
colId,
api,
pinnedLeft,
pinnedRight,
invisibleColumns,
isMain,
onVisibleChange,
-}: Params) => {
+}: HeaderMenuProps) => {
const pinColumn = useCallback(
(pinLoc: ColumnPinnedType) => {
api.setColumnsPinned([colId], pinLoc);
},
[api, colId],
);
- const unHideAction = invisibleColumns.length > 0 && (
- <Menu.SubMenu title={t('Unhide')} icon={<Icons.EyeOutlined iconSize="m" />}>
- {invisibleColumns.length > 1 && (
- <Menu.Item
- onClick={() => {
- api.setColumnsVisible(invisibleColumns, true);
- }}
- >
- <b>{t('All %s hidden columns', invisibleColumns.length)}</b>
- </Menu.Item>
- )}
- {invisibleColumns.map(c => (
- <Menu.Item
- key={c.getColId()}
- onClick={() => {
- api.setColumnsVisible([c.getColId()], true);
- }}
- >
- {c.getColDef().headerName}
- </Menu.Item>
- ))}
- </Menu.SubMenu>
- );
+ const unHideAction: MenuItem = {
+ label: t('Unhide'),
+ key: 'unHideSubMenu',
+ icon: <Icons.EyeInvisibleOutlined iconSize="m" />,
+ children: [
+ invisibleColumns.length > 1 && {
+ key: 'allHidden',
+ label: <b>{t('All %s hidden columns', invisibleColumns.length)}</b>,
+ onClick: () => {
+ api.setColumnsVisible(invisibleColumns, true);
+ },
+ },
+ ...invisibleColumns.map(c => ({
+ key: c.getColId(),
+ label: c.getColDef().headerName,
+ onClick: () => {
+ api.setColumnsVisible([c.getColId()], true);
+ },
+ })),
+ ].filter(Boolean) as MenuItem[],
+ };
+
+ const mainMenuItems: MenuItem[] = [
+ {
+ key: 'copyData',
+ label: t('Copy the current data'),
+ icon: <Icons.CopyOutlined iconSize="m" />,
+ onClick: () => {
+ copyTextToClipboard(
+ () =>
+ new Promise((resolve, reject) => {
+ const data = api.getDataAsCsv({
+ columnKeys: api
+ .getAllDisplayedColumns()
+ .map(c => c.getColId())
+ .filter(id => id !== colId),
+ suppressQuotes: true,
+ columnSeparator: '\t',
+ });
+ if (data) {
+ resolve(data);
+ } else {
+ reject();
+ }
+ }),
+ );
+ },
+ },
+ {
+ key: 'downloadCsv',
+ label: t('Download to CSV'),
+ icon: <Icons.DownloadOutlined iconSize="m" />,
+ onClick: () => {
+ api.exportDataAsCsv({
+ columnKeys: api
+ .getAllDisplayedColumns()
+ .map(c => c.getColId())
+ .filter(id => id !== colId),
+ });
+ },
+ },
+ {
+ type: 'divider',
+ },
+ {
+ key: 'autoSizeAllColumns',
+ label: t('Autosize all columns'),
+ icon: <Icons.ColumnWidthOutlined iconSize="m" />,
+ onClick: () => {
+ api.autoSizeAllColumns();
+ },
+ },
+ ];
- if (isMain) {
- return (
- <MenuDotsDropdown
- placement="bottomLeft"
- trigger={['click']}
- onOpenChange={onVisibleChange}
- overlay={
- <Menu style={{ width: 250 }} mode="vertical">
- <Menu.Item
- onClick={() => {
- copyTextToClipboard(
- () =>
- new Promise((resolve, reject) => {
- const data = api.getDataAsCsv({
- columnKeys: api
- .getAllDisplayedColumns()
- .map(c => c.getColId())
- .filter(id => id !== colId),
- suppressQuotes: true,
- columnSeparator: '\t',
- });
- if (data) {
- resolve(data);
- } else {
- reject();
- }
- }),
- );
- }}
- icon={<Icons.CopyOutlined iconSize="m" />}
- >
- {t('Copy the current data')}
- </Menu.Item>
- <Menu.Item
- onClick={() => {
- api.exportDataAsCsv({
- columnKeys: api
- .getAllDisplayedColumns()
- .map(c => c.getColId())
- .filter(id => id !== colId),
- });
- }}
- icon={<Icons.DownloadOutlined iconSize="m" />}
- >
- {t('Download to CSV')}
- </Menu.Item>
- <Menu.Divider />
- <Menu.Item
- onClick={() => {
- api.autoSizeAllColumns();
- }}
- icon={<Icons.ColumnWidthOutlined iconSize="m" />}
- >
- {t('Autosize all columns')}
- </Menu.Item>
- {unHideAction}
- <Menu.Divider />
- <Menu.Item
- onClick={() => {
- api.setColumnsVisible(invisibleColumns, true);
- const columns = api.getColumns();
- if (columns) {
- const pinnedColumns = columns.filter(
- c => c.getColId() !== PIVOT_COL_ID && c.isPinned(),
- );
- api.setColumnsPinned(pinnedColumns, null);
- api.moveColumns(columns, 0);
- const firstColumn = columns.find(
- c => c.getColId() !== PIVOT_COL_ID,
- );
- if (firstColumn) {
- api.ensureColumnVisible(firstColumn, 'start');
- }
- }
- }}
- icon={<IconEmpty className="anticon" />}
- >
- {t('Reset columns')}
- </Menu.Item>
- </Menu>
+ mainMenuItems.push(unHideAction);
+
+ mainMenuItems.push(
+ {
+ type: 'divider',
+ },
+ {
+ key: 'resetColumns',
+ label: t('Reset columns'),
+ icon: <IconEmpty className="anticon" />,
+ onClick: () => {
+ api.setColumnsVisible(invisibleColumns, true);
+ const columns = api.getColumns();
+ if (columns) {
+ const pinnedColumns = columns.filter(
+ c => c.getColId() !== PIVOT_COL_ID && c.isPinned(),
+ );
+ api.setColumnsPinned(pinnedColumns, null);
+ api.moveColumns(columns, 0);
+ const firstColumn = columns.find(c => c.getColId() !== PIVOT_COL_ID);
+ if (firstColumn) {
+ api.ensureColumnVisible(firstColumn, 'start');
+ }
}
- />
- );
+ },
+ },
+ );
+
+ const menuItems: MenuItem[] = [
+ {
+ key: 'copy',
+ label: t('Copy'),
+ icon: <Icons.CopyOutlined iconSize="m" />,
+ onClick: () => {
+ copyTextToClipboard(
+ () =>
+ new Promise((resolve, reject) => {
+ const data = api.getDataAsCsv({
+ columnKeys: [colId],
+ suppressQuotes: true,
+ });
+ if (data) {
+ resolve(data);
+ } else {
+ reject();
Review Comment:
here as well
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Menu): Move to usage of items prop instead of Menu.Item since Menu.Item is deprecated [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #32587:
URL: https://github.com/apache/superset/pull/32587#discussion_r2149931371
##########
superset-frontend/src/components/GridTable/HeaderMenu.tsx:
##########
@@ -41,129 +41,199 @@ type Params = {
onVisibleChange: DropdownProps['onOpenChange'];
};
-const HeaderMenu: React.FC<Params> = ({
+const HeaderMenu: React.FC<HeaderMenuProps> = ({
colId,
api,
pinnedLeft,
pinnedRight,
invisibleColumns,
isMain,
onVisibleChange,
-}: Params) => {
+}: HeaderMenuProps) => {
const pinColumn = useCallback(
(pinLoc: ColumnPinnedType) => {
api.setColumnsPinned([colId], pinLoc);
},
[api, colId],
);
- const unHideAction = invisibleColumns.length > 0 && (
- <Menu.SubMenu title={t('Unhide')} icon={<Icons.EyeOutlined iconSize="m" />}>
- {invisibleColumns.length > 1 && (
- <Menu.Item
- onClick={() => {
- api.setColumnsVisible(invisibleColumns, true);
- }}
- >
- <b>{t('All %s hidden columns', invisibleColumns.length)}</b>
- </Menu.Item>
- )}
- {invisibleColumns.map(c => (
- <Menu.Item
- key={c.getColId()}
- onClick={() => {
- api.setColumnsVisible([c.getColId()], true);
- }}
- >
- {c.getColDef().headerName}
- </Menu.Item>
- ))}
- </Menu.SubMenu>
- );
+ const unHideAction: MenuItem = {
+ label: t('Unhide'),
+ key: 'unHideSubMenu',
+ icon: <Icons.EyeInvisibleOutlined iconSize="m" />,
+ children: [
+ invisibleColumns.length > 1 && {
+ key: 'allHidden',
+ label: <b>{t('All %s hidden columns', invisibleColumns.length)}</b>,
+ onClick: () => {
+ api.setColumnsVisible(invisibleColumns, true);
+ },
+ },
+ ...invisibleColumns.map(c => ({
+ key: c.getColId(),
+ label: c.getColDef().headerName,
+ onClick: () => {
+ api.setColumnsVisible([c.getColId()], true);
+ },
+ })),
+ ].filter(Boolean) as MenuItem[],
+ };
+
+ const mainMenuItems: MenuItem[] = [
+ {
+ key: 'copyData',
+ label: t('Copy the current data'),
+ icon: <Icons.CopyOutlined iconSize="m" />,
+ onClick: () => {
+ copyTextToClipboard(
+ () =>
+ new Promise((resolve, reject) => {
+ const data = api.getDataAsCsv({
+ columnKeys: api
+ .getAllDisplayedColumns()
+ .map(c => c.getColId())
+ .filter(id => id !== colId),
+ suppressQuotes: true,
+ columnSeparator: '\t',
+ });
+ if (data) {
+ resolve(data);
+ } else {
+ reject();
+ }
+ }),
+ );
+ },
+ },
+ {
+ key: 'downloadCsv',
+ label: t('Download to CSV'),
+ icon: <Icons.DownloadOutlined iconSize="m" />,
+ onClick: () => {
+ api.exportDataAsCsv({
+ columnKeys: api
+ .getAllDisplayedColumns()
+ .map(c => c.getColId())
+ .filter(id => id !== colId),
+ });
+ },
+ },
+ {
+ type: 'divider',
+ },
+ {
+ key: 'autoSizeAllColumns',
+ label: t('Autosize all columns'),
+ icon: <Icons.ColumnWidthOutlined iconSize="m" />,
+ onClick: () => {
+ api.autoSizeAllColumns();
+ },
+ },
+ ];
- if (isMain) {
- return (
- <MenuDotsDropdown
- placement="bottomLeft"
- trigger={['click']}
- onOpenChange={onVisibleChange}
- overlay={
- <Menu style={{ width: 250 }} mode="vertical">
- <Menu.Item
- onClick={() => {
- copyTextToClipboard(
- () =>
- new Promise((resolve, reject) => {
- const data = api.getDataAsCsv({
- columnKeys: api
- .getAllDisplayedColumns()
- .map(c => c.getColId())
- .filter(id => id !== colId),
- suppressQuotes: true,
- columnSeparator: '\t',
- });
- if (data) {
- resolve(data);
- } else {
- reject();
- }
- }),
- );
- }}
- icon={<Icons.CopyOutlined iconSize="m" />}
- >
- {t('Copy the current data')}
- </Menu.Item>
- <Menu.Item
- onClick={() => {
- api.exportDataAsCsv({
- columnKeys: api
- .getAllDisplayedColumns()
- .map(c => c.getColId())
- .filter(id => id !== colId),
- });
- }}
- icon={<Icons.DownloadOutlined iconSize="m" />}
- >
- {t('Download to CSV')}
- </Menu.Item>
- <Menu.Divider />
- <Menu.Item
- onClick={() => {
- api.autoSizeAllColumns();
- }}
- icon={<Icons.ColumnWidthOutlined iconSize="m" />}
- >
- {t('Autosize all columns')}
- </Menu.Item>
- {unHideAction}
- <Menu.Divider />
- <Menu.Item
- onClick={() => {
- api.setColumnsVisible(invisibleColumns, true);
- const columns = api.getColumns();
- if (columns) {
- const pinnedColumns = columns.filter(
- c => c.getColId() !== PIVOT_COL_ID && c.isPinned(),
- );
- api.setColumnsPinned(pinnedColumns, null);
- api.moveColumns(columns, 0);
- const firstColumn = columns.find(
- c => c.getColId() !== PIVOT_COL_ID,
- );
- if (firstColumn) {
- api.ensureColumnVisible(firstColumn, 'start');
- }
- }
- }}
- icon={<IconEmpty className="anticon" />}
- >
- {t('Reset columns')}
- </Menu.Item>
- </Menu>
+ mainMenuItems.push(unHideAction);
+
+ mainMenuItems.push(
+ {
+ type: 'divider',
+ },
+ {
+ key: 'resetColumns',
+ label: t('Reset columns'),
+ icon: <IconEmpty className="anticon" />,
+ onClick: () => {
+ api.setColumnsVisible(invisibleColumns, true);
+ const columns = api.getColumns();
+ if (columns) {
+ const pinnedColumns = columns.filter(
+ c => c.getColId() !== PIVOT_COL_ID && c.isPinned(),
+ );
+ api.setColumnsPinned(pinnedColumns, null);
+ api.moveColumns(columns, 0);
+ const firstColumn = columns.find(c => c.getColId() !== PIVOT_COL_ID);
+ if (firstColumn) {
+ api.ensureColumnVisible(firstColumn, 'start');
+ }
}
- />
- );
+ },
+ },
+ );
+
+ const menuItems: MenuItem[] = [
+ {
+ key: 'copy',
+ label: t('Copy'),
+ icon: <Icons.CopyOutlined iconSize="m" />,
+ onClick: () => {
+ copyTextToClipboard(
+ () =>
+ new Promise((resolve, reject) => {
+ const data = api.getDataAsCsv({
+ columnKeys: [colId],
+ suppressQuotes: true,
+ });
+ if (data) {
+ resolve(data);
+ } else {
+ reject();
Review Comment:
Yeah, i haven't touched the functionality.
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Menu): Move to usage of items prop instead of Menu.Item since Menu.Item is deprecated [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #32587:
URL: https://github.com/apache/superset/pull/32587#issuecomment-2713209098
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment `/korbit-review`.
> Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Menu): Use items prop instead of deprecated Menu.Item HOC [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #32587:
URL: https://github.com/apache/superset/pull/32587#discussion_r2150572986
##########
superset-frontend/src/components/Dropdown/index.tsx:
##########
@@ -82,7 +82,7 @@ export enum IconOrientation {
}
export interface MenuDotsDropdownProps extends AntdDropdownProps {
- overlay: ReactElement;
+ overlay?: ReactElement;
Review Comment:
overlay prop can still be used but i made it optional
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Menu): Move to usage of items prop instead of Menu.Item since Menu.Item is deprecated [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #32587:
URL: https://github.com/apache/superset/pull/32587#discussion_r2149929730
##########
superset-frontend/src/components/GridTable/HeaderMenu.tsx:
##########
@@ -41,129 +41,199 @@ type Params = {
onVisibleChange: DropdownProps['onOpenChange'];
};
-const HeaderMenu: React.FC<Params> = ({
+const HeaderMenu: React.FC<HeaderMenuProps> = ({
colId,
api,
pinnedLeft,
pinnedRight,
invisibleColumns,
isMain,
onVisibleChange,
-}: Params) => {
+}: HeaderMenuProps) => {
const pinColumn = useCallback(
(pinLoc: ColumnPinnedType) => {
api.setColumnsPinned([colId], pinLoc);
},
[api, colId],
);
- const unHideAction = invisibleColumns.length > 0 && (
- <Menu.SubMenu title={t('Unhide')} icon={<Icons.EyeOutlined iconSize="m" />}>
- {invisibleColumns.length > 1 && (
- <Menu.Item
- onClick={() => {
- api.setColumnsVisible(invisibleColumns, true);
- }}
- >
- <b>{t('All %s hidden columns', invisibleColumns.length)}</b>
- </Menu.Item>
- )}
- {invisibleColumns.map(c => (
- <Menu.Item
- key={c.getColId()}
- onClick={() => {
- api.setColumnsVisible([c.getColId()], true);
- }}
- >
- {c.getColDef().headerName}
- </Menu.Item>
- ))}
- </Menu.SubMenu>
- );
+ const unHideAction: MenuItem = {
+ label: t('Unhide'),
+ key: 'unHideSubMenu',
+ icon: <Icons.EyeInvisibleOutlined iconSize="m" />,
+ children: [
+ invisibleColumns.length > 1 && {
+ key: 'allHidden',
+ label: <b>{t('All %s hidden columns', invisibleColumns.length)}</b>,
+ onClick: () => {
+ api.setColumnsVisible(invisibleColumns, true);
+ },
+ },
+ ...invisibleColumns.map(c => ({
+ key: c.getColId(),
+ label: c.getColDef().headerName,
+ onClick: () => {
+ api.setColumnsVisible([c.getColId()], true);
+ },
+ })),
+ ].filter(Boolean) as MenuItem[],
+ };
+
+ const mainMenuItems: MenuItem[] = [
+ {
+ key: 'copyData',
+ label: t('Copy the current data'),
+ icon: <Icons.CopyOutlined iconSize="m" />,
+ onClick: () => {
+ copyTextToClipboard(
+ () =>
+ new Promise((resolve, reject) => {
+ const data = api.getDataAsCsv({
+ columnKeys: api
+ .getAllDisplayedColumns()
+ .map(c => c.getColId())
+ .filter(id => id !== colId),
+ suppressQuotes: true,
+ columnSeparator: '\t',
+ });
+ if (data) {
+ resolve(data);
+ } else {
+ reject();
Review Comment:
I didn't touch this part of the code, these were just moved over to items prop.
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Menu): Use items prop instead of deprecated Menu.Item HOC [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev merged PR #32587:
URL: https://github.com/apache/superset/pull/32587
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Menu): Move to usage of items prop instead of Menu.Item since Menu.Item is deprecated [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #32587:
URL: https://github.com/apache/superset/pull/32587#discussion_r2149973622
##########
superset-frontend/src/components/GridTable/HeaderMenu.tsx:
##########
@@ -41,129 +41,199 @@ type Params = {
onVisibleChange: DropdownProps['onOpenChange'];
};
-const HeaderMenu: React.FC<Params> = ({
+const HeaderMenu: React.FC<HeaderMenuProps> = ({
colId,
api,
pinnedLeft,
pinnedRight,
invisibleColumns,
isMain,
onVisibleChange,
-}: Params) => {
+}: HeaderMenuProps) => {
const pinColumn = useCallback(
(pinLoc: ColumnPinnedType) => {
api.setColumnsPinned([colId], pinLoc);
},
[api, colId],
);
- const unHideAction = invisibleColumns.length > 0 && (
- <Menu.SubMenu title={t('Unhide')} icon={<Icons.EyeOutlined iconSize="m" />}>
- {invisibleColumns.length > 1 && (
- <Menu.Item
- onClick={() => {
- api.setColumnsVisible(invisibleColumns, true);
- }}
- >
- <b>{t('All %s hidden columns', invisibleColumns.length)}</b>
- </Menu.Item>
- )}
- {invisibleColumns.map(c => (
- <Menu.Item
- key={c.getColId()}
- onClick={() => {
- api.setColumnsVisible([c.getColId()], true);
- }}
- >
- {c.getColDef().headerName}
- </Menu.Item>
- ))}
- </Menu.SubMenu>
- );
+ const unHideAction: MenuItem = {
+ label: t('Unhide'),
+ key: 'unHideSubMenu',
+ icon: <Icons.EyeInvisibleOutlined iconSize="m" />,
+ children: [
+ invisibleColumns.length > 1 && {
+ key: 'allHidden',
+ label: <b>{t('All %s hidden columns', invisibleColumns.length)}</b>,
+ onClick: () => {
+ api.setColumnsVisible(invisibleColumns, true);
+ },
+ },
+ ...invisibleColumns.map(c => ({
+ key: c.getColId(),
+ label: c.getColDef().headerName,
+ onClick: () => {
+ api.setColumnsVisible([c.getColId()], true);
+ },
+ })),
+ ].filter(Boolean) as MenuItem[],
+ };
+
+ const mainMenuItems: MenuItem[] = [
+ {
+ key: 'copyData',
+ label: t('Copy the current data'),
+ icon: <Icons.CopyOutlined iconSize="m" />,
+ onClick: () => {
+ copyTextToClipboard(
+ () =>
+ new Promise((resolve, reject) => {
+ const data = api.getDataAsCsv({
+ columnKeys: api
+ .getAllDisplayedColumns()
+ .map(c => c.getColId())
+ .filter(id => id !== colId),
+ suppressQuotes: true,
+ columnSeparator: '\t',
+ });
+ if (data) {
+ resolve(data);
+ } else {
+ reject();
Review Comment:
As you prefer, making small improvements or adjustments as we develop, as in this case, is beneficial; it helps maintain a cleaner codebase with minimal effort.
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Menu): Use items prop instead of deprecated Menu.Item HOC [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #32587:
URL: https://github.com/apache/superset/pull/32587#discussion_r2150587231
##########
superset-frontend/src/components/Dropdown/index.tsx:
##########
@@ -82,7 +82,7 @@ export enum IconOrientation {
}
export interface MenuDotsDropdownProps extends AntdDropdownProps {
- overlay: ReactElement;
+ overlay?: ReactElement;
Review Comment:
Isnβt it already included in AntdDropdownProps?
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Menu): Use items prop instead of deprecated Menu.Item HOC [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #32587:
URL: https://github.com/apache/superset/pull/32587#discussion_r2150620549
##########
superset-frontend/src/components/GridTable/HeaderMenu.tsx:
##########
@@ -41,129 +41,199 @@ type Params = {
onVisibleChange: DropdownProps['onOpenChange'];
};
-const HeaderMenu: React.FC<Params> = ({
+const HeaderMenu: React.FC<HeaderMenuProps> = ({
colId,
api,
pinnedLeft,
pinnedRight,
invisibleColumns,
isMain,
onVisibleChange,
-}: Params) => {
+}: HeaderMenuProps) => {
const pinColumn = useCallback(
(pinLoc: ColumnPinnedType) => {
api.setColumnsPinned([colId], pinLoc);
},
[api, colId],
);
- const unHideAction = invisibleColumns.length > 0 && (
- <Menu.SubMenu title={t('Unhide')} icon={<Icons.EyeOutlined iconSize="m" />}>
- {invisibleColumns.length > 1 && (
- <Menu.Item
- onClick={() => {
- api.setColumnsVisible(invisibleColumns, true);
- }}
- >
- <b>{t('All %s hidden columns', invisibleColumns.length)}</b>
- </Menu.Item>
- )}
- {invisibleColumns.map(c => (
- <Menu.Item
- key={c.getColId()}
- onClick={() => {
- api.setColumnsVisible([c.getColId()], true);
- }}
- >
- {c.getColDef().headerName}
- </Menu.Item>
- ))}
- </Menu.SubMenu>
- );
+ const unHideAction: MenuItem = {
+ label: t('Unhide'),
+ key: 'unHideSubMenu',
+ icon: <Icons.EyeInvisibleOutlined iconSize="m" />,
+ children: [
+ invisibleColumns.length > 1 && {
+ key: 'allHidden',
+ label: <b>{t('All %s hidden columns', invisibleColumns.length)}</b>,
+ onClick: () => {
+ api.setColumnsVisible(invisibleColumns, true);
+ },
+ },
+ ...invisibleColumns.map(c => ({
+ key: c.getColId(),
+ label: c.getColDef().headerName,
+ onClick: () => {
+ api.setColumnsVisible([c.getColId()], true);
+ },
+ })),
+ ].filter(Boolean) as MenuItem[],
+ };
+
+ const mainMenuItems: MenuItem[] = [
+ {
+ key: 'copyData',
+ label: t('Copy the current data'),
+ icon: <Icons.CopyOutlined iconSize="m" />,
+ onClick: () => {
+ copyTextToClipboard(
+ () =>
+ new Promise((resolve, reject) => {
+ const data = api.getDataAsCsv({
+ columnKeys: api
+ .getAllDisplayedColumns()
+ .map(c => c.getColId())
+ .filter(id => id !== colId),
+ suppressQuotes: true,
+ columnSeparator: '\t',
+ });
+ if (data) {
+ resolve(data);
+ } else {
+ reject();
Review Comment:
I see your point but i rather stick to the purpose of the PR, especially for this one. Maybe we can open a follow up pr to add error messages on rejected promises here and a few other places.
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org