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

feat(editor): Add setting for using editor with expressionInterpreter enabled #1461

Merged
merged 2 commits into from
Jan 27, 2025

Conversation

hydrosquall
Copy link
Member

@hydrosquall hydrosquall commented Jan 26, 2025

Motivation

Changes

Before After
image image
  • Adds an explicit dependency on vega-interpreter to package.json
  • Adds expressionInterpreter to the root redux state. (Open to naming this so it looks more like a boolean, but since the name is long already I figured this was OK to start), and wires up associated Redux boilerplate (reducers, action creators, etc)
  • Adds a settings toggle to the sidepanel (pictured)
  • When I saved the files, some extra formatting was applied by my VSCode defaults. At first I was going to revert the changes, but then I saw the styles match what you get when running yarn format $(modified_files), so I left them as is.

Testing

@hydrosquall hydrosquall force-pushed the cameron.yick/vega-interpreter-control branch from b9917c9 to 6689d95 Compare January 26, 2025 16:51
Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

Thank you. Looks great.

Comment on lines 146 to 151
if (mode === Mode.VegaLite) {
// In vl mode, we compile Vega-Lite spec along with config to Vega spec
runtime = vega.parse(vegaSpec);
runtime = vega.parse(vegaSpec, {}, parseOptions);
} else {
runtime = vega.parse(vegaSpec, config as VgConfig);
runtime = vega.parse(vegaSpec, config as VgConfig, parseOptions);
}
Copy link
Member

Choose a reason for hiding this comment

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

Could probably simplify this to mode === Mode.VegaLite ? {} : config

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, just pushed the change 👍

@hydrosquall hydrosquall force-pushed the cameron.yick/vega-interpreter-control branch from 6689d95 to 8dffdc7 Compare January 27, 2025 01:05
@hydrosquall hydrosquall self-assigned this Jan 27, 2025
@hydrosquall
Copy link
Member Author

CI not passing seems to be unrelated to this diff (I opened a separate issue for it #1462 ).

Would you recommend merging the new setting first, or doing the CI fix first?

@domoritz
Copy link
Member

I'm okay with either. Having a ci fix first would be nicer since then we can merge this one cleanly.

@hydrosquall hydrosquall force-pushed the cameron.yick/vega-interpreter-control branch from 8dffdc7 to a6ea2ca Compare January 27, 2025 17:50
@hydrosquall
Copy link
Member Author

Fixed CI in #1463, merging this since the checks are green.

@hydrosquall hydrosquall merged commit 7947363 into master Jan 27, 2025
1 check passed
@hydrosquall hydrosquall deleted the cameron.yick/vega-interpreter-control branch January 27, 2025 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Allow running graph preview with expressionInterpreter mode enabled
2 participants