-
Notifications
You must be signed in to change notification settings - Fork 14.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Shows related dashboards in Explore #21685
feat: Shows related dashboards in Explore #21685
Conversation
...t-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.test.tsx
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #21685 +/- ##
==========================================
+ Coverage 66.82% 66.83% +0.01%
==========================================
Files 1799 1800 +1
Lines 68884 68937 +53
Branches 7319 7335 +16
==========================================
+ Hits 46031 46077 +46
- Misses 20966 20967 +1
- Partials 1887 1893 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
66dee30
to
02a89a6
Compare
02a89a6
to
b0c5e8f
Compare
/testenv up FEATURE_CROSS_REFERENCES=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some non-blocking nits from looking at the code. I will manually test as well
superset-frontend/src/explore/components/ExploreChartHeader/index.jsx
Outdated
Show resolved
Hide resolved
const dashboards = []; | ||
for (let i = 1; i <= numberOfItems; i += 1) { | ||
dashboards.push({ id: i, dashboard_title: `Dashboard ${i}` }); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a shortcut if you like
const dashboards = []; | |
for (let i = 1; i <= numberOfItems; i += 1) { | |
dashboards.push({ id: i, dashboard_title: `Dashboard ${i}` }); | |
} | |
const dashboards = Array.from({ length: numberOfItems }, (_, i) => ({ id: i+1, dashboard_title: `Dashboard ${i+1}`})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
return ( | ||
<> | ||
{showSearch && ( | ||
<AntdInput |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "enhance it"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just move it in its own components directory and wrap it with styled components. As I said, it's just a plus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the code to use our styled component from src/components/Input
👍🏼
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/DashboardsSubMenu.tsx
Show resolved
Hide resolved
</> | ||
)} | ||
)} | ||
{isFeatureEnabled(FeatureFlag.CROSS_REFERENCES) && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/testenv up FEATURE_CROSS_REFERENCES=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT!
Ephemeral environment shutdown and build artifacts deleted. |
SUMMARY
Adds a new section to the "More" button in Explore that displays a list of dashboards in which the chart is included.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Screen.Recording.2022-10-03.at.9.56.43.AM.mov
TESTING INSTRUCTIONS
Check the video and the related tests for instructions.
ADDITIONAL INFORMATION