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

Web console: new Ace, diff view, and cleanup. Decorating the console for the holidays ✨ 🎁 #12085

Merged
merged 7 commits into from
Dec 23, 2021

Conversation

vogievetsky
Copy link
Contributor

@vogievetsky vogievetsky commented Dec 21, 2021

At a high level this PR adds a DiffDialog to visualize the change in supervisor specs between edits. This PR also upgrades the Ace editor (the main code editor for all the JSONs and SQLs). This is done by upgrading to the latest version or react-ace which in turn changes how the Ace editor itself is imported from using brace to using ace-builds (see the readme of react-ace for more details on why).

New Diff view:

image

Updated auto suggestion styling to be more on brand:

image

So here are the changes that generate the diffs:

  • Switched from brace to ace-builds which generates a large number of dependent package and license changes
  • Updated to new ReactAce and went everywhere updating the APIs (there are many changes on the JS and the CSS endpoints)
  • Normalized the capitalization and spelling of all the describe labels
  • Pulled out all the Ace editor styles into a single file ace.scss

This PR has:

  • been self-reviewed.
  • been tested in a test Druid cluster.

@FrankChen021
Copy link
Member

Do we need to handle the HTML tags in the function description?

image

@vogievetsky
Copy link
Contributor Author

Great catch!

@vogievetsky
Copy link
Contributor Author

This is even a bug on master!

image

@vogievetsky
Copy link
Contributor Author

Fixed and updated the screenshot in the description

@vogievetsky
Copy link
Contributor Author

Just realized that I updated the description with a screenshot for a different function than the one we were talking about, so here it is also:

image

Copy link
Member

@clintropolis clintropolis left a comment

Choose a reason for hiding this comment

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

lgtm, pulled locally and clicked around a bit, supervisor diff stuff is pretty cool 👍

@vogievetsky vogievetsky merged commit 37112d2 into apache:master Dec 23, 2021
@vogievetsky vogievetsky deleted the diffs branch December 23, 2021 00:31
@abhishekagarwal87 abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants