-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Vis: Default editor] EUIficate Vega options tab #47473
Conversation
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, tested and didn't notice any problems
src/legacy/core_plugins/vis_type_vega/public/components/vega_vis_editor.tsx
Outdated
Show resolved
Hide resolved
src/legacy/core_plugins/vis_type_vega/public/components/vega_vis_editor.tsx
Outdated
Show resolved
Hide resolved
Pinging @elastic/kibana-app (Team:KibanaApp) |
💚 Build Succeeded |
💚 Build Succeeded |
@elasticmachine merge upstream |
@snide @chandlerprall @cchaos @ryankeairns |
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, but we need to leave some configuration for reformatting. Please take a look at the comments below.
src/legacy/core_plugins/vis_type_vega/public/components/vega_help_menu.tsx
Outdated
Show resolved
Hide resolved
src/legacy/core_plugins/vis_type_vega/public/components/vega_help_menu.tsx
Outdated
Show resolved
Hide resolved
src/legacy/core_plugins/vis_type_vega/public/components/vega_help_menu.tsx
Outdated
Show resolved
Hide resolved
function format(value: string, stringify: typeof compactStringify) { | ||
try { | ||
const spec = hjson.parse(value, { legacyRoot: false, keepWsc: true }); | ||
return stringify(spec); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can skip options like it was before https://github.com/elastic/kibana/pull/47473/files#diff-2abd10d2a2ed5649151d832d6ae32983L63.
For example without options reformatting as HJON removes comments:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Fixed.
But currently HJSON
formatting works incorrectly because of upgrading -> #48023
src/legacy/core_plugins/vis_type_vega/public/components/vega_help_menu.tsx
Show resolved
Hide resolved
💔 Build Failed |
@elasticmachine merge upstream |
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a couple regressions due to some of the CSS removals.
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* EUIficate Vega options tab * Update list of dependencies * Fix comments * Adjust styles
Summary
A part of #38247.
EUIfication of the
Vega
visualization editor.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers