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.

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(explore): Fill dashboard name when adding new chart from dashboard view #20129

Merged
merged 4 commits into from
May 31, 2022

Conversation

kgabryje
Copy link
Member

SUMMARY

When user enters chart creation flow starting from a dashboard (e.g. with "Create chart" button in empty state or charts list in dashboard edit mode), the chart's save modal should have that dashboard's name prefilled. To achieve that, we pass dashboard_id as a url param to /chart/add page and from there to explore.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2022-05-19.at.12.43.24.mov

TESTING INSTRUCTIONS

  1. Open a dashboard -> edit mode -> charts tab -> click "Create new chart" OR Open a dashboard and create a new chart using the plus icon button in top right corner.
  2. Create a chart and click save
  3. Verify that dashboard's name is prefilled
  4. Create a new dashboard and click "Create new chart" button in empty state. Create a chart and verify that dashboard's name is prefilled in save modal
  5. Create a chart from any other view (for example from Welcome page). Verify that save modal is not prefilled

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • 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

CC @kasiazjc

@@ -699,12 +699,17 @@ function mapStateToProps(state) {
const chartKey = Object.keys(charts)[0];
const chart = charts[chartKey];

let dashboardId = Number(explore.form_data?.dashboardId);
if (Number.isNaN(dashboardId)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

?? operator doesn't exclude NaNs. I also don't want to use || operator in order not to exclude dashboards with id 0

@codecov
Copy link

codecov bot commented May 19, 2022

Codecov Report

Merging #20129 (b016a37) into master (c8fe518) will decrease coverage by 0.00%.
The diff coverage is 36.36%.

@@            Coverage Diff             @@
##           master   #20129      +/-   ##
==========================================
- Coverage   66.47%   66.47%   -0.01%     
==========================================
  Files        1727     1727              
  Lines       64724    64731       +7     
  Branches     6822     6824       +2     
==========================================
+ Hits        43024    43028       +4     
- Misses      19969    19970       +1     
- Partials     1731     1733       +2     
Flag Coverage Δ
javascript 51.34% <36.36%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rontend/src/dashboard/components/DashboardGrid.jsx 62.50% <0.00%> (ø)
...t-frontend/src/dashboard/components/SliceAdder.jsx 66.17% <0.00%> (ø)
...nd/src/dashboard/components/gridComponents/Tab.jsx 83.67% <ø> (ø)
...rontend/src/dashboard/containers/DashboardGrid.jsx 100.00% <ø> (ø)
...t-frontend/src/dashboard/containers/SliceAdder.jsx 0.00% <ø> (ø)
...rset-frontend/src/explore/components/SaveModal.tsx 66.19% <ø> (ø)
...perset-frontend/src/views/components/MenuRight.tsx 59.79% <33.33%> (-0.85%) ⬇️
...perset-frontend/src/addSlice/AddSliceContainer.tsx 61.11% <50.00%> (-0.66%) ⬇️
.../explore/components/ExploreViewContainer/index.jsx 52.84% <66.66%> (+0.54%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8fe518...b016a37. Read the comment docs.

@kgabryje kgabryje force-pushed the feat/prefill-dashboard-name branch from 99de199 to b016a37 Compare May 31, 2022 11:50
Copy link
Member

@AAfghahi AAfghahi left a comment

Choose a reason for hiding this comment

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

Looks good! Just some questions, no feedback

const formData = encodeURIComponent(
JSON.stringify({
viz_type: this.state.visType,
datasource: this.state.datasource?.value,
...(!isNullish(dashboardId) && { dashboardId }),
Copy link
Member

Choose a reason for hiding this comment

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

this is a cool function, why nullish?

Copy link
Member Author

Choose a reason for hiding this comment

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

!dashboardId would exclude dashboardId === 0, which is a valid id 😄

@@ -19,6 +19,7 @@
/* header has mysterious extra margin */
header.top {
margin-bottom: 2px;
z-index: 10;
Copy link
Member

Choose a reason for hiding this comment

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

why is this necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

The main menu items on the right were not clickable because in edit mode the right panel was covering them (they were visible but unclickable because of invisible part of that panel covering them)

Copy link
Member Author

Choose a reason for hiding this comment

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

You can test that by opening dashboard in edit mode and then try to click the "+" icon in top right corner

@AAfghahi
Copy link
Member

/testenv up

@AAfghahi AAfghahi added the need:qa-review Requires QA review label May 31, 2022
@github-actions
Copy link
Contributor

@AAfghahi Ephemeral environment spinning up at http://34.221.68.91:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@kgabryje kgabryje merged commit 3e3fbcc into apache:master May 31, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
…rd view (apache#20129)

* feat(explore): Fill dashboard name when adding new chart from dashboard view

* Update import paths

* Update test

* Fix test
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels need:qa-review Requires QA review preset-io size/M 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants