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: broken glyphicons used in react-json-schema #10267

Merged
merged 1 commit into from
Jul 14, 2020

Conversation

mistercrunch
Copy link
Member

@mistercrunch mistercrunch commented Jul 9, 2020

I'm working on a talk for the Airflow Summit
"Advanced Apache Superset for Data Engineers" and showing the "Schedule
Query" feature that Beto contributed a while back (behind a feature flag).

I found that the glyphicons used in react-json-schema are broken and
came up with an easy fix.

Also other minor tweaks on the feature.

before

Screen Shot 2020-07-08 at 10 34 34 PM

after

Screen Shot 2020-07-08 at 10 28 44 PM

Screen Shot 2020-07-08 at 10 28 32 PM

@mistercrunch mistercrunch added the sqllab Namespace | Anything related to the SQL Lab label Jul 9, 2020
@mistercrunch mistercrunch force-pushed the scheduled-queries branch 2 times, most recently from a4cd08e to 42bb6b7 Compare July 9, 2020 05:51
@codecov-commenter
Copy link

codecov-commenter commented Jul 9, 2020

Codecov Report

Merging #10267 into master will decrease coverage by 4.89%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10267      +/-   ##
==========================================
- Coverage   70.18%   65.28%   -4.90%     
==========================================
  Files         598      598              
  Lines       32004    31994      -10     
  Branches     3236     3236              
==========================================
- Hits        22462    20888    -1574     
- Misses       9437    10925    +1488     
- Partials      105      181      +76     
Flag Coverage Δ
#cypress ?
#javascript 59.28% <ø> (ø)
#python 69.46% <ø> (-0.22%) ⬇️
Impacted Files Coverage Δ
...tend/src/SqlLab/components/ScheduleQueryButton.jsx 8.62% <ø> (ø)
superset/views/sql_lab.py 62.57% <ø> (ø)
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/SqlLab/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupColors.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/chart/ChartContainer.jsx 0.00% <0.00%> (-100.00%) ⬇️
... and 154 more

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 3dfa4aa...42bb6b7. Read the comment docs.

@ktmud
Copy link
Member

ktmud commented Jul 9, 2020

On a separate note, we can probably start thinking about replacing all code editor instances with monaco-editor, the editor behind VS Code, which also supports json schemas. It's much more actively maintained than Ace/Brace and has many more advanced features.

I'm working on a talk for the Airflow Summit
"Advanced Apache Superset for Data Engineers" and showing the "Schedule
Query" feature that Beto contributed a while back (behind a feature flag).

I found that the glyphicons used in `react-json-schema` are broken and
came up with an easy fix.

Also other minor tweaks on the feature.
@@ -162,6 +162,7 @@ class SavedQueryViewApi(SavedQueryView): # pylint: disable=too-many-ancestors
"description",
"sql",
"extra_json",
"extra",
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised to see both extra and extra_json - what's the difference/motivation?

Copy link
Member Author

Choose a reason for hiding this comment

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

extra_json is a string containing the serialized JSON, extra is the actual nested data structure. While I know the private API does not use extra_json, I assume that people in the wild (notably at Lyft where @betodealmeida shipped the feature) are very likely to use it.

Clearly it's not great to have JSON inception (serialized JSON inside of JSON)

I could have deprecated extra_json in favor of extra, but would need to do change management around it - likely just adding a note in UPDATING.md. Eventually we should move this under /api/v1 but, maybe a good time to do this.

@rusackas rusackas self-requested a review July 14, 2020 20:50
@mistercrunch mistercrunch merged commit 11ae480 into apache:master Jul 14, 2020
@mistercrunch mistercrunch deleted the scheduled-queries branch July 14, 2020 22:41
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
I'm working on a talk for the Airflow Summit
"Advanced Apache Superset for Data Engineers" and showing the "Schedule
Query" feature that Beto contributed a while back (behind a feature flag).

I found that the glyphicons used in `react-json-schema` are broken and
came up with an easy fix.

Also other minor tweaks on the feature.
@rumbin rumbin mentioned this pull request Mar 27, 2021
@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 sqllab Namespace | Anything related to the SQL Lab 🚢 0.38.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants