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

Enable beautiful json for dashboard metadata editor #9474

Closed
wants to merge 1 commit into from

Conversation

ktmud
Copy link
Member

@ktmud ktmud commented Apr 6, 2020

CATEGORY

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

Reformat JSON strings in the dashboard metadata modal input so it's easier to edit.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

Snip20200405_58

After

Snip20200405_57

TEST PLAN

  1. Pick a random dashboard with filters
  2. Edit metadata via "Edit dashboard" -> "" -> "Edit dashboard properties" -> "Advanced".

The JSON string in the code editor should be formatted.

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

REVIEWERS

@graceguo-supercat @etr2460

@@ -111,6 +111,7 @@
"interweave": "^11.2.0",
"jquery": "^3.4.1",
"json-bigint": "^0.3.0",
"json-stringify-pretty-compact": "^2.0.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

There's no change to package-lock.json because this package is already included by vega-lite (run npm ls json-stringify-pretty-compact):

├─┬ @superset-ui/preset-chart-xy@0.11.16
│ └─┬ vega-lite@4.1.1
│   └── json-stringify-pretty-compact@2.0.0  deduped
└── json-stringify-pretty-compact@2.0.0 

@@ -99,7 +108,8 @@ class PropertiesModal extends React.PureComponent {
...state.values,
dashboard_title: dashboard.dashboard_title || '',
slug: dashboard.slug || '',
json_metadata: dashboard.json_metadata || '',
// always reformat to pretty json at initial opening
json_metadata: prettyJSON(dashboard.json_metadata) || '',
Copy link
Member Author

Choose a reason for hiding this comment

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

Only reformat the initial value so not to interfere users' typing.

@@ -281,7 +291,6 @@ class PropertiesModal extends React.PureComponent {
<AceEditor
mode="json"
name="json_metadata"
defaultValue={this.defaultMetadataValue}
Copy link
Member Author

Choose a reason for hiding this comment

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

defaultMetadataValue was not found anywhere. Seems to be uncleaned old code.

@ktmud ktmud force-pushed the dashboard-meta-beautify-json branch from dad3c64 to 472e7d1 Compare April 6, 2020 22:16
@ktmud
Copy link
Member Author

ktmud commented Apr 7, 2020

Close to trigger CI

@ktmud ktmud closed this Apr 7, 2020
@@ -24,6 +24,7 @@ import Select from 'react-select';
import AceEditor from 'react-ace';
import { t } from '@superset-ui/translation';
import { SupersetClient } from '@superset-ui/connection';
import stringify from 'json-stringify-pretty-compact';
Copy link
Member

Choose a reason for hiding this comment

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

what are the advantages to using this module over the native JSON.stringify function with formatting (3rd arg)?

Copy link
Member Author

@ktmud ktmud Apr 7, 2020

Choose a reason for hiding this comment

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

This generates more compact strings.

E.g.

{
  "short array": [1, 2, 3],
  "long array": [
    {"x": 1, "y": 2},
    {"x": 2, "y": 1},
    {"x": 1, "y": 1},
    {"x": 2, "y": 2}
  ]
}

vs

{
  "short array": [
    1,
    2,
    3
  ],
  "long array": [
    {
      "x": 1,
      "y": 2
    },
    {
      "x": 2,
      "y": 1
    },
    {
      "x": 1,
      "y": 1
    },
    {
      "x": 2,
      "y": 2
    }
  ]
}

Copy link
Member

Choose a reason for hiding this comment

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

ah, hence "compact" in the name 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants