feat(dashboard): add dashboard description field in modal and apis

[opus-42]

SUMMARY

This pull request add a dashboard description field in the dashboard APIs and in the dashboard modal UI. This evolution is part of an effort to bring documentation of Superset object, discussion about this issue can be found in the following issue: #14510

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

• [X] Has associated issue: #14510
• [ ] Required feature flags:
• [x] 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

[codecov[bot]]

Codecov Report

Merging #17203 (824dc71) into master (8c52fe3) will decrease coverage by 10.67%. The diff coverage is n/a.

:exclamation: Current head 824dc71 differs from pull request most recent head 85d3154. Consider uploading reports for the commit 85d3154 to get more accurate results

@@             Coverage Diff             @@
##           master   #17203       +/-   ##
===========================================
- Coverage   66.52%   55.84%   -10.68%     
===========================================
  Files        1641     1831      +190     
  Lines       63476    70002     +6526     
  Branches     6444     7570     +1126     
===========================================
- Hits        42227    39094     -3133     
- Misses      19585    28950     +9365     
- Partials     1664     1958      +294     

Flag	Coverage Δ
hive	52.88% <ø> (+0.28%)	:arrow_up:
javascript	53.77% <ø> (+2.48%)	:arrow_up:
mysql	?
postgres	?
presto	52.78% <ø> (+0.34%)	:arrow_up:
python	58.09% <ø> (-24.20%)	:arrow_down:
sqlite	?
unit	51.26% <ø> (?)

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

Impacted Files	Coverage Δ
...ntrols/src/components/CertifiedIconWithTooltip.tsx	80.00% <ø> (ø)
...-ui-chart-controls/src/components/ColumnOption.tsx	85.71% <ø> (ø)
...src/components/ColumnTypeLabel/ColumnTypeLabel.tsx	100.00% <ø> (ø)
...controls/src/components/InfoTooltipWithTrigger.tsx	100.00% <ø> (ø)
...-ui-chart-controls/src/components/MetricOption.tsx	90.90% <ø> (-3.83%)	:arrow_down:
...et-ui-chart-controls/src/components/SQLPopover.tsx	100.00% <ø> (ø)
...erset-ui-chart-controls/src/components/Tooltip.tsx	80.00% <ø> (ø)
...et-ui-chart-controls/src/components/labelUtils.tsx	100.00% <ø> (+10.52%)	:arrow_up:
...ckages/superset-ui-chart-controls/src/constants.ts	100.00% <ø> (ø)
...ackages/superset-ui-chart-controls/src/fixtures.ts	100.00% <ø> (ø)
... and 1210 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[geido]

Hello @opus-42 thanks for the contribution. It appears that you have some linting issues. Would you please fix those so that I can spin up a test env? Thank you!

[villebro]

@opus-42 I love this addition! It would also be great to add this as a tooltip next to the dashboard name on the Dashboard list view when defined. What do you think?

[opus-42]

Hello @villebro, yes I think this is a great addition. I was not sure what the next step are (tooltip, additional field in the list, addition to the Dashboard UI...) and could be a good way to do it. Let me take a look at how to do it, or it can be kept for another pull request so that the display format can be discussed separately. What do you think ?

[opus-42]

Here is a screenshot of last state of the pull request with a TextArea.

NB: Linting errors should be resolved.

[opus-42]

Hello sorry, I had no time to get back to that feature, trying now to resolve the last front-end test that was blocking, linked to change in modal format. Should be hopefully good now (tested locally) ! Thks

[opus-42]

Hello @riahk, thank you ! There was some merge conflicts with master. Could you check out everything is still fine after the merge ? Thanks again

[riahk]

@opus-42 It looks like your spec for PropertiesModal needs to be updated! A Certification section got added that needs to be accounted for. :) Other than that it still looks good!

[opus-42]

@riahk Thank you, I'll finalise this pull request now so that it can be integrated in subsequent versions.

[opus-42]

Hello @riahk. Could you check if everything is ok to be merged ? (CI/CD needs review) Thanks 🙏

[geido]

Hello @opus-42. Thank you for moving this forward! I see there's is a conflict. Would you mind checking that out?

Also, if you are interested in contributing more to Superset and haven't already, join the Superset Slack . I go with the same username over there. I'll be glad to help you!

[geido]

/testenv up

[github-actions[bot]]

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

[geido]

Hello @opus-42 below some comments after manual testing your PR

• When adding a description, applying, saving and then reopening the properties modal again, the description isn't appearing. This happens also when the Dashboard is reloaded, making me think it is not being saved.

https://user-images.githubusercontent.com/60598000/152997097-3a9f1bd9-bab9-4429-ac25-9a31262d18d4.mp4

• What do you think of showing the description in the Dashboards list in a tooltip just after each Dashboard title?

[opus-42]

• When adding a description, applying, saving and then reopening the properties modal again, the description isn't appearing

Ok, this seems only to happen in the dashboard Edit Mode. I'll patch that.

• What do you think of showing the description in the Dashboards list in a tooltip just after each Dashboard title? Yes that would be a great addition. I'll look at it also.

[opus-42]

@geido

Here is a proposition for the tooltip

Or maybe we could put the tool tip after the link (with SupersetTheme color as well)

NB: when put after there is some slight Icon shift on overing due to Tooltip default style

.ant-tooltip-open {
            display: inline-block;
            &::after {
              content: '';
              display: block;
            }
         }

[geido]

@geido

Here is a proposition for the tooltip

Or maybe we could put the tool tip after the link (with SupersetTheme color as well)

NB: when put after there is some slight Icon shift on overing due to Tooltip default style

.ant-tooltip-open {
            display: inline-block;
            &::after {
              content: '';
              display: block;
            }
         }

Hello @opus-42 thanks for the adding. I think the icon on the right is the proper place as it is consistent with the certification icon. I am running CI and spinning up a test env for more testing

[opus-42]

@geido Here is a proposition for the tooltip Or maybe we could put the tool tip after the link (with SupersetTheme color as well) NB: when put after there is some slight Icon shift on overing due to Tooltip default style

.ant-tooltip-open {
            display: inline-block;
            &::after {
              content: '';
              display: block;
            }
         }

Hello @opus-42 thanks for the adding. I think the icon on the right is the proper place as it is consistent with the certification icon. I am running CI and spinning up a test env for more testing

Thks. Let me finalize first, I haven't pushed the changes yet + test on my side, I'll tag you then.

[opus-42]

Hello @geido.

https://user-images.githubusercontent.com/33355870/156941000-4818c661-a6c0-4cf8-bb8b-17634c368026.mov

I have completed checks and addition of the tooltip for dashboard description. See the screen record below. All test have passed and I think everything should now be finally ready !

[geido]

/testenv up

[github-actions[bot]]

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

[opus-42]

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

I have just test, I see a refresh issue when changing the description (tooltip does not seems to change). Pushing the fix now. Should be as in the screen recording.

[kylepapili]

Any remaining blockers on getting this PR merged? Would love to take advantage of this feature.

[villebro]

Any remaining blockers on getting this PR merged? Would love to take advantage of this feature.

Hello @opus-42 - sorry for letting this slip through the cracks. Can you rebase the PR to resolve the conflicts? I'm happy to help push this through.

[opus-42]

Hello @villebro, happy to get back and finalise this as well, but it need to be rebased and somewhat re-written I think following the recent frontend evolution. Let me get through the rebase 😃

[rusackas]

I'm for some reason no able to re-start the CI actions on this PR, which I hope are just flaky. I'm going to close and re-open this in hopes that it kick-starts things! 🤞

[github-actions[bot]]

Ephemeral environment shutdown and build artifacts deleted.

[rusackas]

@opus-42 it's been a while since this PR saw any love, but I think it's safe to say we're all still quite interested in getting this feature merged, if you're able to rebase the PR and help it across the finish line 🤞❤️

[one-data-cookie]

@michael-s-molina Isn't it a bit of shame to close this, since it's been almost done? Or are there any plans to implement it via a different PR?

[michael-s-molina]

@michael-s-molina Isn't it a bit of shame to close this, since it's been almost done? Or are there any plans to implement it via a different PR?

Yes, it is but the PR is stale as you can see by @rusackas comment that was not replied:

@opus-42 it's been a while since this PR saw any love, but I think it's safe to say we're all still quite interested in getting this feature merged, if you're able to rebase the PR and help it across the finish line 🤞❤️

@one-data-cookie Are you interested in reopening the PR and work on it?