You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/10/05 20:07:55 UTC
[GitHub] [superset] michael-s-molina opened a new pull request, #21685: feat: Shows related dashboards in Explore
michael-s-molina opened a new pull request, #21685:
URL: https://github.com/apache/superset/pull/21685
### SUMMARY
Adds a new section to the "More" button in Explore that displays a list of dashboards in which the chart is included.
@kasiazjc I'll treat the chart autofocus when navigating as a separate PR because there are things to consider like tabs, scrolling, etc.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
https://user-images.githubusercontent.com/70410625/193631237-268d5acf-a2f7-41a8-83a5-5a97b478f62e.mov
### TESTING INSTRUCTIONS
Check the video and the related tests for instructions.
### ADDITIONAL INFORMATION
- [ ] 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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #21685: feat: Shows related dashboards in Explore
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21685:
URL: https://github.com/apache/superset/pull/21685#discussion_r989362220
##########
superset-frontend/src/dashboard/actions/hydrate.js:
##########
@@ -291,8 +292,26 @@ export const hydrateDashboard =
future: [],
};
+ // Searches for a focused_chart parameter in the URL to automatically focus a chart
+ const { focused_chart: focusedChartId } = regularUrlParams;
+ let focusedChartLayoutId;
+ if (focusedChartId) {
+ // Converts focused_chart to dashboard layout id
+ const found = Object.values(dashboardLayout.present).find(
+ // eslint-disable-next-line eqeqeq
Review Comment:
Done
--
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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #21685: feat: Shows related dashboards in Explore
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21685:
URL: https://github.com/apache/superset/pull/21685#discussion_r989275506
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.jsx:
##########
@@ -246,14 +249,25 @@ export const useExploreAdditionalActionsMenu = (
openKeys={openSubmenus}
onOpenChange={setOpenSubmenus}
>
- {slice && (
- <>
+ <>
+ {slice && (
<Menu.Item key={MENU_KEYS.EDIT_PROPERTIES}>
{t('Edit chart properties')}
</Menu.Item>
- <Menu.Divider />
- </>
- )}
+ )}
+ {isFeatureEnabled(FeatureFlag.CROSS_REFERENCES) && (
Review Comment:
The design specification shows the "Dashboards added to" with a `None` item. I think this is intentional to present the user with the same experience when there's no dashboard associated whether is a new chart or a saved one.
<img width="442" alt="Screen Shot 2022-10-06 at 1 43 13 PM" src="https://user-images.githubusercontent.com/70410625/194371000-b1fa250f-1f77-4842-b9d4-0917ec5b9bc1.png">
--
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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #21685: feat: Shows related dashboards in Explore
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21685:
URL: https://github.com/apache/superset/pull/21685#discussion_r989268930
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.tsx:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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 React, { useState } from 'react';
+import { css, t, useTheme } from '@superset-ui/core';
+import { AntdInput } from 'src/components';
+import Icons from 'src/components/Icons';
+import { Menu } from 'src/components/Menu';
+import { Link } from 'react-router-dom';
+
+export interface DashboardsSubMenuProps {
+ chartId?: number;
+ dashboards?: { id: number; dashboard_title: string }[];
+}
+
+const WIDTH = 220;
+const HEIGHT = 300;
+const SEARCH_THRESHOLD = 10;
+
+const DashboardsSubMenu = ({
+ chartId,
+ dashboards = [],
+ ...menuProps
+}: DashboardsSubMenuProps) => {
+ const theme = useTheme();
+ const [dashboardSearch, setDashboardSearch] = useState<string>();
+ const [hoveredItem, setHoveredItem] = useState<number | null>();
+ const showSearch = dashboards.length > SEARCH_THRESHOLD;
+ const filteredDashboards = dashboards.filter(
+ dashboard =>
+ !dashboardSearch ||
+ dashboard.dashboard_title
+ .toLowerCase()
+ .includes(dashboardSearch.toLowerCase()),
+ );
+ const noResults = dashboards.length === 0;
+ const noResultsFound = dashboardSearch && filteredDashboards.length === 0;
+ const urlQueryString = chartId ? `?focused_chart=${chartId}` : '';
+ return (
+ <>
+ {showSearch && (
+ <AntdInput
+ allowClear
+ placeholder={t('Search')}
+ prefix={<Icons.Search iconSize="l" />}
+ css={css`
+ width: ${WIDTH}px;
+ margin: ${theme.gridUnit * 2}px ${theme.gridUnit * 3}px;
+ `}
+ value={dashboardSearch}
+ onChange={e => setDashboardSearch(e.currentTarget.value?.trim())}
+ />
+ )}
+ <div
+ css={css`
+ max-height: ${HEIGHT}px;
+ overflow: auto;
+ `}
+ >
+ {filteredDashboards.map(dashboard => (
+ <Menu.Item
+ key={`${dashboard.id}`}
+ onMouseEnter={() => setHoveredItem(dashboard.id)}
+ onMouseLeave={() => {
+ if (hoveredItem === dashboard.id) {
+ setHoveredItem(null);
+ }
+ }}
+ {...menuProps}
+ >
+ <Link
+ target="_blank"
Review Comment:
Nice catch! Done.
--
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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #21685: feat: Shows related dashboards in Explore
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21685:
URL: https://github.com/apache/superset/pull/21685#discussion_r990067197
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.tsx:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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 React, { useState } from 'react';
+import { css, t, useTheme } from '@superset-ui/core';
+import { AntdInput } from 'src/components';
+import Icons from 'src/components/Icons';
+import { Menu } from 'src/components/Menu';
+import { Link } from 'react-router-dom';
+
+export interface DashboardsSubMenuProps {
+ chartId?: number;
+ dashboards?: { id: number; dashboard_title: string }[];
+}
+
+const WIDTH = 220;
+const HEIGHT = 300;
+const SEARCH_THRESHOLD = 10;
+
+const DashboardsSubMenu = ({
+ chartId,
+ dashboards = [],
+ ...menuProps
+}: DashboardsSubMenuProps) => {
+ const theme = useTheme();
+ const [dashboardSearch, setDashboardSearch] = useState<string>();
+ const [hoveredItem, setHoveredItem] = useState<number | null>();
+ const showSearch = dashboards.length > SEARCH_THRESHOLD;
+ const filteredDashboards = dashboards.filter(
+ dashboard =>
+ !dashboardSearch ||
+ dashboard.dashboard_title
+ .toLowerCase()
+ .includes(dashboardSearch.toLowerCase()),
+ );
+ const noResults = dashboards.length === 0;
+ const noResultsFound = dashboardSearch && filteredDashboards.length === 0;
+ const urlQueryString = chartId ? `?focused_chart=${chartId}` : '';
+ return (
+ <>
+ {showSearch && (
+ <AntdInput
Review Comment:
I changed the code to use our styled `Input` from `src/components` 👍🏼
--
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
[GitHub] [superset] kgabryje commented on a diff in pull request #21685: feat: Shows related dashboards in Explore
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #21685:
URL: https://github.com/apache/superset/pull/21685#discussion_r989186762
##########
superset-frontend/src/dashboard/actions/hydrate.js:
##########
@@ -291,8 +292,26 @@ export const hydrateDashboard =
future: [],
};
+ // Searches for a focused_chart parameter in the URL to automatically focus a chart
+ const { focused_chart: focusedChartId } = regularUrlParams;
+ let focusedChartLayoutId;
+ if (focusedChartId) {
+ // Converts focused_chart to dashboard layout id
+ const found = Object.values(dashboardLayout.present).find(
+ // eslint-disable-next-line eqeqeq
Review Comment:
eqeqeq? 😄
##########
superset-frontend/src/explore/components/ExploreChartHeader/index.jsx:
##########
@@ -162,6 +165,13 @@ export const ExploreChartHeader = ({
metadata.dashboards.length > 0
? t('Added to %s dashboard(s)', metadata.dashboards.length)
: t('Not added to any dashboard'),
+ description:
+ metadata.dashboards.length > 0 &&
+ isFeatureEnabled(FeatureFlag.CROSS_REFERENCES)
Review Comment:
Why do we lock it behind a feature flag? (sorry if I missed that conversation)
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.jsx:
##########
@@ -246,14 +249,25 @@ export const useExploreAdditionalActionsMenu = (
openKeys={openSubmenus}
onOpenChange={setOpenSubmenus}
>
- {slice && (
- <>
+ <>
+ {slice && (
<Menu.Item key={MENU_KEYS.EDIT_PROPERTIES}>
{t('Edit chart properties')}
</Menu.Item>
- <Menu.Divider />
- </>
- )}
+ )}
+ {isFeatureEnabled(FeatureFlag.CROSS_REFERENCES) && (
Review Comment:
this should be behind the `slice &&` check too. If you create a new chart, slice is undefined before saving, so there's no point in showing "Dashboards added to" because it's always empty. If you do that, you can also make `chartId` required instead of optional in types of `DashboardSubMenu`
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.tsx:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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 React, { useState } from 'react';
+import { css, t, useTheme } from '@superset-ui/core';
+import { AntdInput } from 'src/components';
+import Icons from 'src/components/Icons';
+import { Menu } from 'src/components/Menu';
+import { Link } from 'react-router-dom';
+
+export interface DashboardsSubMenuProps {
+ chartId?: number;
+ dashboards?: { id: number; dashboard_title: string }[];
+}
+
+const WIDTH = 220;
+const HEIGHT = 300;
+const SEARCH_THRESHOLD = 10;
+
+const DashboardsSubMenu = ({
+ chartId,
+ dashboards = [],
+ ...menuProps
+}: DashboardsSubMenuProps) => {
+ const theme = useTheme();
+ const [dashboardSearch, setDashboardSearch] = useState<string>();
+ const [hoveredItem, setHoveredItem] = useState<number | null>();
+ const showSearch = dashboards.length > SEARCH_THRESHOLD;
+ const filteredDashboards = dashboards.filter(
+ dashboard =>
+ !dashboardSearch ||
+ dashboard.dashboard_title
+ .toLowerCase()
+ .includes(dashboardSearch.toLowerCase()),
+ );
+ const noResults = dashboards.length === 0;
+ const noResultsFound = dashboardSearch && filteredDashboards.length === 0;
+ const urlQueryString = chartId ? `?focused_chart=${chartId}` : '';
+ return (
+ <>
+ {showSearch && (
+ <AntdInput
+ allowClear
+ placeholder={t('Search')}
+ prefix={<Icons.Search iconSize="l" />}
+ css={css`
+ width: ${WIDTH}px;
+ margin: ${theme.gridUnit * 2}px ${theme.gridUnit * 3}px;
+ `}
+ value={dashboardSearch}
+ onChange={e => setDashboardSearch(e.currentTarget.value?.trim())}
+ />
+ )}
+ <div
+ css={css`
+ max-height: ${HEIGHT}px;
+ overflow: auto;
+ `}
+ >
+ {filteredDashboards.map(dashboard => (
+ <Menu.Item
+ key={`${dashboard.id}`}
+ onMouseEnter={() => setHoveredItem(dashboard.id)}
+ onMouseLeave={() => {
+ if (hoveredItem === dashboard.id) {
+ setHoveredItem(null);
+ }
+ }}
+ {...menuProps}
+ >
+ <Link
+ target="_blank"
Review Comment:
```suggestion
target="_blank" rel="noreferer noopener"
```
--
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
[GitHub] [superset] geido commented on a diff in pull request #21685: feat: Shows related dashboards in Explore
Posted by GitBox <gi...@apache.org>.
geido commented on code in PR #21685:
URL: https://github.com/apache/superset/pull/21685#discussion_r989879366
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.tsx:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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 React, { useState } from 'react';
+import { css, t, useTheme } from '@superset-ui/core';
+import { AntdInput } from 'src/components';
+import Icons from 'src/components/Icons';
+import { Menu } from 'src/components/Menu';
+import { Link } from 'react-router-dom';
+
+export interface DashboardsSubMenuProps {
+ chartId?: number;
+ dashboards?: { id: number; dashboard_title: string }[];
+}
+
+const WIDTH = 220;
+const HEIGHT = 300;
+const SEARCH_THRESHOLD = 10;
+
+const DashboardsSubMenu = ({
+ chartId,
+ dashboards = [],
+ ...menuProps
+}: DashboardsSubMenuProps) => {
+ const theme = useTheme();
+ const [dashboardSearch, setDashboardSearch] = useState<string>();
+ const [hoveredItem, setHoveredItem] = useState<number | null>();
+ const showSearch = dashboards.length > SEARCH_THRESHOLD;
+ const filteredDashboards = dashboards.filter(
+ dashboard =>
+ !dashboardSearch ||
+ dashboard.dashboard_title
+ .toLowerCase()
+ .includes(dashboardSearch.toLowerCase()),
+ );
+ const noResults = dashboards.length === 0;
+ const noResultsFound = dashboardSearch && filteredDashboards.length === 0;
+ const urlQueryString = chartId ? `?focused_chart=${chartId}` : '';
+ return (
+ <>
+ {showSearch && (
+ <AntdInput
Review Comment:
Just move it in its own components directory and wrap it with styled components. As I said, it's just a plus
--
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
[GitHub] [superset] kgabryje commented on a diff in pull request #21685: feat: Shows related dashboards in Explore
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #21685:
URL: https://github.com/apache/superset/pull/21685#discussion_r989325861
##########
superset-frontend/src/dashboard/actions/hydrate.js:
##########
@@ -291,8 +292,26 @@ export const hydrateDashboard =
future: [],
};
+ // Searches for a focused_chart parameter in the URL to automatically focus a chart
+ const { focused_chart: focusedChartId } = regularUrlParams;
+ let focusedChartLayoutId;
+ if (focusedChartId) {
+ // Converts focused_chart to dashboard layout id
+ const found = Object.values(dashboardLayout.present).find(
+ // eslint-disable-next-line eqeqeq
Review Comment:
Why do we need to use `==`? Are meta.chartId and focusedChartId different types? In that case we could try casting to number. Alternatively we could add focused_chart to URL_PARAMS as number type and retrieve it with getUrlParam - it would be cast to number automatically
--
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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #21685: feat: Shows related dashboards in Explore
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21685:
URL: https://github.com/apache/superset/pull/21685#discussion_r989267591
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.tsx:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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 React, { useState } from 'react';
+import { css, t, useTheme } from '@superset-ui/core';
+import { AntdInput } from 'src/components';
+import Icons from 'src/components/Icons';
+import { Menu } from 'src/components/Menu';
+import { Link } from 'react-router-dom';
+
+export interface DashboardsSubMenuProps {
+ chartId?: number;
+ dashboards?: { id: number; dashboard_title: string }[];
+}
+
+const WIDTH = 220;
+const HEIGHT = 300;
+const SEARCH_THRESHOLD = 10;
+
+const DashboardsSubMenu = ({
+ chartId,
+ dashboards = [],
+ ...menuProps
+}: DashboardsSubMenuProps) => {
+ const theme = useTheme();
+ const [dashboardSearch, setDashboardSearch] = useState<string>();
+ const [hoveredItem, setHoveredItem] = useState<number | null>();
+ const showSearch = dashboards.length > SEARCH_THRESHOLD;
+ const filteredDashboards = dashboards.filter(
+ dashboard =>
+ !dashboardSearch ||
+ dashboard.dashboard_title
+ .toLowerCase()
+ .includes(dashboardSearch.toLowerCase()),
+ );
+ const noResults = dashboards.length === 0;
+ const noResultsFound = dashboardSearch && filteredDashboards.length === 0;
+ const urlQueryString = chartId ? `?focused_chart=${chartId}` : '';
+ return (
+ <>
+ {showSearch && (
+ <AntdInput
Review Comment:
What do you mean by "enhance it"?
--
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
[GitHub] [superset] kgabryje commented on pull request #21685: feat: Shows related dashboards in Explore
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #21685:
URL: https://github.com/apache/superset/pull/21685#issuecomment-1270265622
/testenv up FEATURE_CROSS_REFERENCES=true
--
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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #21685: feat: Shows related dashboards in Explore
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21685:
URL: https://github.com/apache/superset/pull/21685#discussion_r989263138
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.test.tsx:
##########
@@ -0,0 +1,78 @@
+/**
+ * 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 React from 'react';
+import { render, screen, waitFor } from 'spec/helpers/testing-library';
+import userEvent from '@testing-library/user-event';
+import { Menu } from 'src/components/Menu';
+import DashboardItems from './DashboardsSubMenu';
+
+const asyncRender = (numberOfItems: number) =>
+ waitFor(() => {
+ const dashboards = [];
+ for (let i = 1; i <= numberOfItems; i += 1) {
+ dashboards.push({ id: i, dashboard_title: `Dashboard ${i}` });
+ }
Review Comment:
Interesting! For this specific case though, after formatting, both options take 4 lines of code and your suggestion requires more knowledge about iterable objects with the `length` property. I think the original code is more developer friendly in this case. Nevertheless, I liked the suggested pattern and will use it in other cases.
--
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
[GitHub] [superset] michael-s-molina merged pull request #21685: feat: Shows related dashboards in Explore
Posted by GitBox <gi...@apache.org>.
michael-s-molina merged PR #21685:
URL: https://github.com/apache/superset/pull/21685
--
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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #21685: feat: Shows related dashboards in Explore
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21685:
URL: https://github.com/apache/superset/pull/21685#discussion_r989246316
##########
superset-frontend/src/dashboard/actions/hydrate.js:
##########
@@ -291,8 +292,26 @@ export const hydrateDashboard =
future: [],
};
+ // Searches for a focused_chart parameter in the URL to automatically focus a chart
+ const { focused_chart: focusedChartId } = regularUrlParams;
+ let focusedChartLayoutId;
+ if (focusedChartId) {
+ // Converts focused_chart to dashboard layout id
+ const found = Object.values(dashboardLayout.present).find(
+ // eslint-disable-next-line eqeqeq
Review Comment:
It made me laugh too! 🤣
--
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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #21685: feat: Shows related dashboards in Explore
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21685:
URL: https://github.com/apache/superset/pull/21685#discussion_r989278278
##########
superset-frontend/src/explore/components/ExploreChartHeader/index.jsx:
##########
@@ -162,6 +165,13 @@ export const ExploreChartHeader = ({
metadata.dashboards.length > 0
? t('Added to %s dashboard(s)', metadata.dashboards.length)
: t('Not added to any dashboard'),
+ description:
+ metadata.dashboards.length > 0 &&
+ isFeatureEnabled(FeatureFlag.CROSS_REFERENCES)
Review Comment:
The description is displayed in a tooltip when you hover the dashboards element in the metadata bar. It displays "You can preview the list of dashboards on the chart settings dropdown". If the feature flag is disabled, there's no option in the chart settings dropdown 😛
--
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
[GitHub] [superset] kgabryje commented on pull request #21685: feat: Shows related dashboards in Explore
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #21685:
URL: https://github.com/apache/superset/pull/21685#issuecomment-1271394649
/testenv up FEATURE_CROSS_REFERENCES=true
--
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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #21685: feat: Shows related dashboards in Explore
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21685:
URL: https://github.com/apache/superset/pull/21685#discussion_r990067197
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.tsx:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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 React, { useState } from 'react';
+import { css, t, useTheme } from '@superset-ui/core';
+import { AntdInput } from 'src/components';
+import Icons from 'src/components/Icons';
+import { Menu } from 'src/components/Menu';
+import { Link } from 'react-router-dom';
+
+export interface DashboardsSubMenuProps {
+ chartId?: number;
+ dashboards?: { id: number; dashboard_title: string }[];
+}
+
+const WIDTH = 220;
+const HEIGHT = 300;
+const SEARCH_THRESHOLD = 10;
+
+const DashboardsSubMenu = ({
+ chartId,
+ dashboards = [],
+ ...menuProps
+}: DashboardsSubMenuProps) => {
+ const theme = useTheme();
+ const [dashboardSearch, setDashboardSearch] = useState<string>();
+ const [hoveredItem, setHoveredItem] = useState<number | null>();
+ const showSearch = dashboards.length > SEARCH_THRESHOLD;
+ const filteredDashboards = dashboards.filter(
+ dashboard =>
+ !dashboardSearch ||
+ dashboard.dashboard_title
+ .toLowerCase()
+ .includes(dashboardSearch.toLowerCase()),
+ );
+ const noResults = dashboards.length === 0;
+ const noResultsFound = dashboardSearch && filteredDashboards.length === 0;
+ const urlQueryString = chartId ? `?focused_chart=${chartId}` : '';
+ return (
+ <>
+ {showSearch && (
+ <AntdInput
Review Comment:
I changed the code to use our styled `Input` from `src/components/Input` 👍🏼
--
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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #21685: feat: Shows related dashboards in Explore
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21685:
URL: https://github.com/apache/superset/pull/21685#discussion_r990067197
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.tsx:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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 React, { useState } from 'react';
+import { css, t, useTheme } from '@superset-ui/core';
+import { AntdInput } from 'src/components';
+import Icons from 'src/components/Icons';
+import { Menu } from 'src/components/Menu';
+import { Link } from 'react-router-dom';
+
+export interface DashboardsSubMenuProps {
+ chartId?: number;
+ dashboards?: { id: number; dashboard_title: string }[];
+}
+
+const WIDTH = 220;
+const HEIGHT = 300;
+const SEARCH_THRESHOLD = 10;
+
+const DashboardsSubMenu = ({
+ chartId,
+ dashboards = [],
+ ...menuProps
+}: DashboardsSubMenuProps) => {
+ const theme = useTheme();
+ const [dashboardSearch, setDashboardSearch] = useState<string>();
+ const [hoveredItem, setHoveredItem] = useState<number | null>();
+ const showSearch = dashboards.length > SEARCH_THRESHOLD;
+ const filteredDashboards = dashboards.filter(
+ dashboard =>
+ !dashboardSearch ||
+ dashboard.dashboard_title
+ .toLowerCase()
+ .includes(dashboardSearch.toLowerCase()),
+ );
+ const noResults = dashboards.length === 0;
+ const noResultsFound = dashboardSearch && filteredDashboards.length === 0;
+ const urlQueryString = chartId ? `?focused_chart=${chartId}` : '';
+ return (
+ <>
+ {showSearch && (
+ <AntdInput
Review Comment:
I changed the code to use our styled component from `src/components/Input` 👍🏼
--
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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #21685: feat: Shows related dashboards in Explore
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21685:
URL: https://github.com/apache/superset/pull/21685#discussion_r989264208
##########
superset-frontend/src/explore/components/ExploreChartHeader/index.jsx:
##########
@@ -162,6 +165,13 @@ export const ExploreChartHeader = ({
metadata.dashboards.length > 0
? t('Added to %s dashboard(s)', metadata.dashboards.length)
: t('Not added to any dashboard'),
+ description:
+ metadata.dashboards.length > 0 &&
+ isFeatureEnabled(FeatureFlag.CROSS_REFERENCES)
+ ? t(
+ 'To preview the list of dashboards go to "more" settings on the right.',
Review Comment:
Make sense. I'll change it to your suggestion. Pinging @kasiazjc for awareness since the text came from the design screens.
--
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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #21685: feat: Shows related dashboards in Explore
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21685:
URL: https://github.com/apache/superset/pull/21685#discussion_r989362220
##########
superset-frontend/src/dashboard/actions/hydrate.js:
##########
@@ -291,8 +292,26 @@ export const hydrateDashboard =
future: [],
};
+ // Searches for a focused_chart parameter in the URL to automatically focus a chart
+ const { focused_chart: focusedChartId } = regularUrlParams;
+ let focusedChartLayoutId;
+ if (focusedChartId) {
+ // Converts focused_chart to dashboard layout id
+ const found = Object.values(dashboardLayout.present).find(
+ // eslint-disable-next-line eqeqeq
Review Comment:
Yes, they have different types. I followed your suggestion and used `getUrlParam` instead.
--
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
[GitHub] [superset] geido commented on a diff in pull request #21685: feat: Shows related dashboards in Explore
Posted by GitBox <gi...@apache.org>.
geido commented on code in PR #21685:
URL: https://github.com/apache/superset/pull/21685#discussion_r989197148
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.test.tsx:
##########
@@ -0,0 +1,78 @@
+/**
+ * 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 React from 'react';
+import { render, screen, waitFor } from 'spec/helpers/testing-library';
+import userEvent from '@testing-library/user-event';
+import { Menu } from 'src/components/Menu';
+import DashboardItems from './DashboardsSubMenu';
+
+const asyncRender = (numberOfItems: number) =>
+ waitFor(() => {
+ const dashboards = [];
+ for (let i = 1; i <= numberOfItems; i += 1) {
+ dashboards.push({ id: i, dashboard_title: `Dashboard ${i}` });
+ }
Review Comment:
Just a shortcut if you like
```suggestion
const dashboards = Array.from({ length: numberOfItems }, (_, i) => ({ id: i+1, dashboard_title: `Dashboard ${i+1}`}));
```
##########
superset-frontend/src/explore/components/ExploreChartHeader/index.jsx:
##########
@@ -162,6 +165,13 @@ export const ExploreChartHeader = ({
metadata.dashboards.length > 0
? t('Added to %s dashboard(s)', metadata.dashboards.length)
: t('Not added to any dashboard'),
+ description:
+ metadata.dashboards.length > 0 &&
+ isFeatureEnabled(FeatureFlag.CROSS_REFERENCES)
+ ? t(
+ 'To preview the list of dashboards go to "more" settings on the right.',
Review Comment:
I think this text can be improved. I wouldn't mention "on the right" as it's easy to lose sight of this if the position should ever change. I believe something more generic like "You can preview the list of dashboards on the chart settings dropdown" or something like that
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.tsx:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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 React, { useState } from 'react';
+import { css, t, useTheme } from '@superset-ui/core';
+import { AntdInput } from 'src/components';
+import Icons from 'src/components/Icons';
+import { Menu } from 'src/components/Menu';
+import { Link } from 'react-router-dom';
+
+export interface DashboardsSubMenuProps {
+ chartId?: number;
+ dashboards?: { id: number; dashboard_title: string }[];
+}
+
+const WIDTH = 220;
+const HEIGHT = 300;
+const SEARCH_THRESHOLD = 10;
+
+const DashboardsSubMenu = ({
+ chartId,
+ dashboards = [],
+ ...menuProps
+}: DashboardsSubMenuProps) => {
+ const theme = useTheme();
+ const [dashboardSearch, setDashboardSearch] = useState<string>();
+ const [hoveredItem, setHoveredItem] = useState<number | null>();
+ const showSearch = dashboards.length > SEARCH_THRESHOLD;
+ const filteredDashboards = dashboards.filter(
+ dashboard =>
+ !dashboardSearch ||
+ dashboard.dashboard_title
+ .toLowerCase()
+ .includes(dashboardSearch.toLowerCase()),
+ );
+ const noResults = dashboards.length === 0;
+ const noResultsFound = dashboardSearch && filteredDashboards.length === 0;
+ const urlQueryString = chartId ? `?focused_chart=${chartId}` : '';
+ return (
+ <>
+ {showSearch && (
+ <AntdInput
+ allowClear
+ placeholder={t('Search')}
+ prefix={<Icons.Search iconSize="l" />}
+ css={css`
+ width: ${WIDTH}px;
+ margin: ${theme.gridUnit * 2}px ${theme.gridUnit * 3}px;
+ `}
+ value={dashboardSearch}
+ onChange={e => setDashboardSearch(e.currentTarget.value?.trim())}
+ />
+ )}
+ <div
+ css={css`
+ max-height: ${HEIGHT}px;
+ overflow: auto;
+ `}
+ >
+ {filteredDashboards.map(dashboard => (
+ <Menu.Item
+ key={`${dashboard.id}`}
Review Comment:
```suggestion
key={String(dashboard.id)}
```
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.tsx:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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 React, { useState } from 'react';
+import { css, t, useTheme } from '@superset-ui/core';
+import { AntdInput } from 'src/components';
+import Icons from 'src/components/Icons';
+import { Menu } from 'src/components/Menu';
+import { Link } from 'react-router-dom';
+
+export interface DashboardsSubMenuProps {
+ chartId?: number;
+ dashboards?: { id: number; dashboard_title: string }[];
+}
+
+const WIDTH = 220;
+const HEIGHT = 300;
+const SEARCH_THRESHOLD = 10;
+
+const DashboardsSubMenu = ({
+ chartId,
+ dashboards = [],
+ ...menuProps
+}: DashboardsSubMenuProps) => {
+ const theme = useTheme();
+ const [dashboardSearch, setDashboardSearch] = useState<string>();
+ const [hoveredItem, setHoveredItem] = useState<number | null>();
+ const showSearch = dashboards.length > SEARCH_THRESHOLD;
+ const filteredDashboards = dashboards.filter(
+ dashboard =>
+ !dashboardSearch ||
+ dashboard.dashboard_title
+ .toLowerCase()
+ .includes(dashboardSearch.toLowerCase()),
+ );
+ const noResults = dashboards.length === 0;
+ const noResultsFound = dashboardSearch && filteredDashboards.length === 0;
+ const urlQueryString = chartId ? `?focused_chart=${chartId}` : '';
+ return (
+ <>
+ {showSearch && (
+ <AntdInput
Review Comment:
Should we move this to its own directory and enhance it like we did for all other Antd components? Definitely not a blocker for the PR though
--
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
[GitHub] [superset] kgabryje commented on a diff in pull request #21685: feat: Shows related dashboards in Explore
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #21685:
URL: https://github.com/apache/superset/pull/21685#discussion_r989325861
##########
superset-frontend/src/dashboard/actions/hydrate.js:
##########
@@ -291,8 +292,26 @@ export const hydrateDashboard =
future: [],
};
+ // Searches for a focused_chart parameter in the URL to automatically focus a chart
+ const { focused_chart: focusedChartId } = regularUrlParams;
+ let focusedChartLayoutId;
+ if (focusedChartId) {
+ // Converts focused_chart to dashboard layout id
+ const found = Object.values(dashboardLayout.present).find(
+ // eslint-disable-next-line eqeqeq
Review Comment:
Why do we need to use `==`? Are meta.chartId and focusedChartId dofferent types? In that case we could try casting to number. Alternatively we could add focused_chart to URL_PARAMS as number type and retrieve it with getUrlParam - it would be cast to number automatically
--
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
[GitHub] [superset] github-actions[bot] commented on pull request #21685: feat: Shows related dashboards in Explore
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21685:
URL: https://github.com/apache/superset/pull/21685#issuecomment-1271775403
Ephemeral environment shutdown and build artifacts deleted.
--
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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #21685: feat: Shows related dashboards in Explore
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21685:
URL: https://github.com/apache/superset/pull/21685#discussion_r989267931
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.tsx:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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 React, { useState } from 'react';
+import { css, t, useTheme } from '@superset-ui/core';
+import { AntdInput } from 'src/components';
+import Icons from 'src/components/Icons';
+import { Menu } from 'src/components/Menu';
+import { Link } from 'react-router-dom';
+
+export interface DashboardsSubMenuProps {
+ chartId?: number;
+ dashboards?: { id: number; dashboard_title: string }[];
+}
+
+const WIDTH = 220;
+const HEIGHT = 300;
+const SEARCH_THRESHOLD = 10;
+
+const DashboardsSubMenu = ({
+ chartId,
+ dashboards = [],
+ ...menuProps
+}: DashboardsSubMenuProps) => {
+ const theme = useTheme();
+ const [dashboardSearch, setDashboardSearch] = useState<string>();
+ const [hoveredItem, setHoveredItem] = useState<number | null>();
+ const showSearch = dashboards.length > SEARCH_THRESHOLD;
+ const filteredDashboards = dashboards.filter(
+ dashboard =>
+ !dashboardSearch ||
+ dashboard.dashboard_title
+ .toLowerCase()
+ .includes(dashboardSearch.toLowerCase()),
+ );
+ const noResults = dashboards.length === 0;
+ const noResultsFound = dashboardSearch && filteredDashboards.length === 0;
+ const urlQueryString = chartId ? `?focused_chart=${chartId}` : '';
+ return (
+ <>
+ {showSearch && (
+ <AntdInput
+ allowClear
+ placeholder={t('Search')}
+ prefix={<Icons.Search iconSize="l" />}
+ css={css`
+ width: ${WIDTH}px;
+ margin: ${theme.gridUnit * 2}px ${theme.gridUnit * 3}px;
+ `}
+ value={dashboardSearch}
+ onChange={e => setDashboardSearch(e.currentTarget.value?.trim())}
+ />
+ )}
+ <div
+ css={css`
+ max-height: ${HEIGHT}px;
+ overflow: auto;
+ `}
+ >
+ {filteredDashboards.map(dashboard => (
+ <Menu.Item
+ key={`${dashboard.id}`}
Review Comment:
Good suggestion. More readable.
--
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