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

fix(dashboard): changing the chart title, except not #10527

Merged
merged 3 commits into from
Aug 8, 2020

Conversation

suddjian
Copy link
Member

@suddjian suddjian commented Aug 5, 2020

SUMMARY

It often makes sense to have a different title for a chart in the context of a dashboard than the title used for the chart on its own. This change allows that. Title changes made from the dashboard will not affect the chart entity, and will instead be saved in the dashboard data.

TEST PLAN

Change the chart title in the dashboard, verify that it does not change the chart title. Change the chart title from explore view, verify that it does not change the custom title in the dashboard.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@suddjian suddjian changed the title Fix/dash change chart title fix(dashboard): changing the chart title, except not Aug 5, 2020
@suddjian suddjian changed the title fix(dashboard): changing the chart title, except not [WiP] fix(dashboard): changing the chart title, except not Aug 5, 2020
@suddjian suddjian changed the title [WiP] fix(dashboard): changing the chart title, except not fix(dashboard): changing the chart title, except not Aug 6, 2020
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Nice, this is much cleaner 👍 I'm wondering if we could introduce a unit test for this, both in ChartHolder_spec.jsx and potentially /tests/dashboards/dao_tests.py?

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Looking at this with new eyes, I feel like adding a test will seem almost weird, as we're essentially just removing mutation logic. No point in adding a test for something that's not happening anymore. LGTM, kudos for net negative LOC PR! 🎉

@suddjian suddjian merged commit 7f84927 into master Aug 8, 2020
@villebro villebro added the v0.38 label Sep 9, 2020
@dpgaspar dpgaspar removed the v0.38 label Sep 10, 2020
Ofeknielsen pushed a commit to ofekisr/incubator-superset that referenced this pull request Oct 5, 2020
* changing slice names in dashboard should not change chart title

* comprehensions > loops
@suddjian suddjian deleted the fix/dash-change-chart-title branch October 31, 2020 04:30
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
* changing slice names in dashboard should not change chart title

* comprehensions > loops
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0 labels Mar 12, 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 size/M 🚢 0.38.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants