Skip to content
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.

Sign up for GitHub

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

chore: move data nav menu to plus menu #18629

Merged
merged 21 commits into from
Feb 17, 2022
Merged

chore: move data nav menu to plus menu #18629

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
681c20e
more data nav menu
pkdotson Feb 9, 2022
a4bbd5e
fix lint and fix nav css
pkdotson Feb 9, 2022
2ae8696
update test and remove icons
pkdotson Feb 9, 2022
237263a
Update superset-frontend/src/views/components/Menu.test.tsx
pkdotson Feb 10, 2022
04d1ff9
Apply suggestions from code review
hughhhh Feb 10, 2022
1c0f383
use backend app.link to show new nav changes
pkdotson Feb 10, 2022
760d8c6
Merge branch 'chore-change-nav' of https://github.com/preset-io/super…
pkdotson Feb 10, 2022
717c5bb
fix lint
pkdotson Feb 10, 2022
2502a9a
update test
pkdotson Feb 10, 2022
123eff6
usetheme and remove chaining
pkdotson Feb 11, 2022
785f74b
add more suggestions
pkdotson Feb 14, 2022
757d502
fix lint
pkdotson Feb 14, 2022
a6c0bbe
add allowed extensions to bootstrap and hard code links
pkdotson Feb 15, 2022
0e9abe9
remove backend links
pkdotson Feb 15, 2022
117261e
fix test
pkdotson Feb 15, 2022
c42491d
add extensions to frontend conf
pkdotson Feb 16, 2022
9eac285
fix test and add be changes
pkdotson Feb 16, 2022
6e9eb0c
test is python test passes
pkdotson Feb 16, 2022
9121e01
update python test and reremove app links
pkdotson Feb 16, 2022
32f6e79
Merge branch 'master' of https://github.com/preset-io/superset into c…
pkdotson Feb 16, 2022
57e001c
fix ts and add t's
pkdotson Feb 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion superset-frontend/src/views/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ import { RootContextProviders } from './RootContextProviders';
setupApp();

const user = { ...bootstrapData.user };
const menu = { ...bootstrapData.common.menu_data };
const menu = {
...bootstrapData.common.menu_data,
};
let lastLocationPathname: string;
initFeatureFlags(bootstrapData.common.feature_flags);

Expand Down
28 changes: 23 additions & 5 deletions superset-frontend/src/views/components/Menu.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,26 @@ const mockedProps = {
url: '/dashboard/list/',
index: 4,
},
{
name: 'Data',
icon: 'fa-database',
label: 'Data',
childs: [
{
name: 'Databases',
icon: 'fa-database',
label: 'Databases',
url: '/databaseview/list/',
},
{
name: 'Datasets',
icon: 'fa-table',
label: 'Datasets',
url: '/tablemodelview/list/',
},
'-',
pkdotson marked this conversation as resolved.
Show resolved Hide resolved
],
},
],
brand: {
path: '/superset/profile/admin/',
Expand Down Expand Up @@ -220,13 +240,11 @@ test('should render the dropdown items', async () => {
render(<Menu {...notanonProps} />);
const dropdown = screen.getByTestId('new-dropdown-icon');
userEvent.hover(dropdown);
expect(await screen.findByText(dropdownItems[0].label)).toHaveAttribute(
// todo (philip): test data submenu
expect(await screen.findByText(dropdownItems[1].label)).toHaveAttribute(
'href',
dropdownItems[0].url,
dropdownItems[1].url,
);
expect(
screen.getByTestId(`menu-item-${dropdownItems[0].label}`),
).toBeInTheDocument();
expect(await screen.findByText(dropdownItems[1].label)).toHaveAttribute(
'href',
dropdownItems[1].url,
Expand Down
36 changes: 21 additions & 15 deletions superset-frontend/src/views/components/Menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/
import React, { useState, useEffect } from 'react';
import { styled, css } from '@superset-ui/core';
import { styled, css, useTheme, SupersetTheme } from '@superset-ui/core';
import { debounce } from 'lodash';
import { Global } from '@emotion/react';
import { getUrlParam } from 'src/utils/urlUtils';
Expand Down Expand Up @@ -74,9 +74,9 @@ export interface MenuProps {

interface MenuObjectChildProps {
label: string;
name?: string;
icon: string;
index: number;
name: string;
icon?: string;
index?: number;
url?: string;
isFrontendRoute?: boolean;
}
Expand Down Expand Up @@ -172,7 +172,21 @@ const StyledHeader = styled.header`
}
}
`;

const globalStyles = (theme: SupersetTheme) => css`
.ant-menu-submenu.ant-menu-submenu-popup.ant-menu.ant-menu-light.ant-menu-submenu-placement-bottomLeft {
border-radius: 0px;
}
.ant-menu-submenu.ant-menu-submenu-popup.ant-menu.ant-menu-light {
border-radius: 0px;
}
.ant-menu-vertical > .ant-menu-submenu.data-menu > .ant-menu-submenu-title {
height: 28px;
i {
padding-right: ${theme.gridUnit * 2}px;
margin-left: ${theme.gridUnit * 1.75}px;
}
}
`;
const { SubMenu } = DropdownMenu;

const { useBreakpoint } = Grid;
Expand All @@ -184,6 +198,7 @@ export function Menu({
const [showMenu, setMenu] = useState<MenuMode>('horizontal');
const screens = useBreakpoint();
const uiConig = useUiConfig();
const theme = useTheme();

useEffect(() => {
function handleResize() {
Expand Down Expand Up @@ -251,16 +266,7 @@ export function Menu({
};
return (
<StyledHeader className="top" id="main-menu" role="navigation">
<Global
styles={css`
.ant-menu-submenu.ant-menu-submenu-popup.ant-menu.ant-menu-light.ant-menu-submenu-placement-bottomLeft {
border-radius: 0px;
}
.ant-menu-submenu.ant-menu-submenu-popup.ant-menu.ant-menu-light {
border-radius: 0px;
}
`}
/>
<Global styles={globalStyles(theme)} />
<Row>
<Col md={16} xs={24}>
<Tooltip
Expand Down
78 changes: 71 additions & 7 deletions superset-frontend/src/views/components/MenuRight.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,38 @@ import { Link } from 'react-router-dom';
import Icons from 'src/components/Icons';
import findPermission from 'src/dashboard/util/findPermission';
import { useSelector } from 'react-redux';
import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes';
import {
UserWithPermissionsAndRoles,
CommonBootstrapData,
} from 'src/types/bootstrapTypes';
import LanguagePicker from './LanguagePicker';
import { NavBarProps, MenuObjectProps } from './Menu';

export const dropdownItems = [
{
pkdotson marked this conversation as resolved.
Show resolved Hide resolved
label: t('Data'),
icon: 'fa-database',
childs: [
{
icon: 'fa-upload',
label: 'Upload a CSV',
pkdotson marked this conversation as resolved.
Show resolved Hide resolved
name: 'Upload a CSV',
url: '/csvtodatabaseview/form',
},
{
icon: 'fa-upload',
label: 'Upload a Columnar File',
name: 'Upload a Columnar file',
url: '/columnartodatabaseview/form',
},
{
icon: 'fa-upload',
label: 'Upload Excel',
name: 'Upload Excel',
url: '/exceltodatabaseview/form',
},
],
},
{
label: t('SQL query'),
url: '/superset/sqllab?new=true',
Expand Down Expand Up @@ -96,12 +123,27 @@ const RightMenu = ({
const { roles } = useSelector<any, UserWithPermissionsAndRoles>(
state => state.user,
);

// @ts-ignore
const { CSV_EXTENSIONS, COLUMNAR_EXTENSIONS, EXCEL_EXTENSIONS } = useSelector<
any,
CommonBootstrapData
>(state => state.common.conf);
// if user has any of these roles the dropdown will appear
const configMap = {
'Upload a CSV': CSV_EXTENSIONS,
'Upload a Columnar file': COLUMNAR_EXTENSIONS,
'Upload Excel': EXCEL_EXTENSIONS,
};
const canSql = findPermission('can_sqllab', 'Superset', roles);
const canDashboard = findPermission('can_write', 'Dashboard', roles);
const canChart = findPermission('can_write', 'Chart', roles);
const showActionDropdown = canSql || canChart || canDashboard;
const menuIconAndLabel = (menu: MenuObjectProps) => (
<>
<i data-test={`menu-item-${menu.label}`} className={`fa ${menu.icon}`} />
{menu.label}
</>
);
return (
<StyledDiv align={align}>
<Menu mode="horizontal">
Expand All @@ -113,9 +155,30 @@ const RightMenu = ({
}
icon={<Icons.TriangleDown />}
>
{dropdownItems.map(
menu =>
findPermission(menu.perm, menu.view, roles) && (
{dropdownItems.map(menu => {
if (menu.childs) {
return (
<SubMenu
key="sub2"
className="data-menu"
title={menuIconAndLabel(menu as MenuObjectProps)}
>
{menu.childs.map(item =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great!

configMap[item.name] === true ? (
<Menu.Item key={item.name}>
<a href={item.url}> {item.label} </a>
</Menu.Item>
) : null,
)}
</SubMenu>
);
}
return (
findPermission(
menu.perm as string,
menu.view as string,
roles,
) && (
<Menu.Item key={menu.label}>
<a href={menu.url}>
<i
Expand All @@ -125,8 +188,9 @@ const RightMenu = ({
{menu.label}
</a>
</Menu.Item>
),
)}
)
);
})}
</SubMenu>
)}
<SubMenu
Expand Down
47 changes: 0 additions & 47 deletions superset/initialization/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,53 +367,6 @@ def init_views(self) -> None:
category_icon="fa-table",
)
appbuilder.add_separator("Data")
appbuilder.add_link(
"Upload a CSV",
label=__("Upload a CSV"),
href="/csvtodatabaseview/form",
icon="fa-upload",
category="Data",
category_label=__("Data"),
category_icon="fa-wrench",
cond=lambda: bool(
self.config["CSV_EXTENSIONS"].intersection(
self.config["ALLOWED_EXTENSIONS"]
)
),
)
appbuilder.add_link(
"Upload a Columnar file",
label=__("Upload a Columnar File"),
href="/columnartodatabaseview/form",
icon="fa-upload",
category="Data",
category_label=__("Data"),
category_icon="fa-wrench",
cond=lambda: bool(
self.config["COLUMNAR_EXTENSIONS"].intersection(
self.config["ALLOWED_EXTENSIONS"]
)
),
)
try:
import xlrd # pylint: disable=unused-import

appbuilder.add_link(
"Upload Excel",
label=__("Upload Excel"),
href="/exceltodatabaseview/form",
icon="fa-upload",
category="Data",
category_label=__("Data"),
category_icon="fa-wrench",
cond=lambda: bool(
self.config["EXCEL_EXTENSIONS"].intersection(
self.config["ALLOWED_EXTENSIONS"]
)
),
)
except ImportError:
pass

appbuilder.add_api(LogRestApi)
appbuilder.add_view(
Expand Down
10 changes: 10 additions & 0 deletions superset/views/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,16 @@ def common_bootstrap_payload() -> Dict[str, Any]:

# should not expose API TOKEN to frontend
frontend_config = {k: conf.get(k) for k in FRONTEND_CONF_KEYS}
frontend_config["EXCEL_EXTENSIONS"] = bool(
bool(conf["EXCEL_EXTENSIONS"].intersection(conf["ALLOWED_EXTENSIONS"])),
)
frontend_config["CSV_EXTENSIONS"] = bool(
bool(conf["CSV_EXTENSIONS"].intersection(conf["ALLOWED_EXTENSIONS"])),
)
frontend_config["COLUMNAR_EXTENSIONS"] = bool(
bool(conf["COLUMNAR_EXTENSIONS"].intersection(conf["ALLOWED_EXTENSIONS"])),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, but to further simplify, wdyt about adding these keys to the FRONTEND_CONF_KEYS and then do the logic on the client side component? In other words, instead of this file being concerned with logic or the usage of this data, it just passes the data to the client and then the client only has to be concerned about the logic.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure to dry that out a bit it makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However I'm going to mark this as a follow up as this will add some testing layer on frontend I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, sounds good. thanks

)

if conf.get("SLACK_API_TOKEN"):
frontend_config["ALERT_REPORTS_NOTIFICATION_METHODS"] = [
ReportRecipientType.EMAIL,
Expand Down
1 change: 0 additions & 1 deletion tests/integration_tests/security_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,6 @@ def assert_can_alpha(self, perm_set):
self.assert_can_menu("Manage", perm_set)
self.assert_can_menu("Annotation Layers", perm_set)
self.assert_can_menu("CSS Templates", perm_set)
self.assert_can_menu("Upload a CSV", perm_set)
self.assertIn(("all_datasource_access", "all_datasource_access"), perm_set)

def assert_cannot_alpha(self, perm_set):
Expand Down