-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add support for advanced/custom config editor #280
Conversation
Your demo site is ready! 🚀 Visit it here: https://ramp4-pcar4.github.io/storylines-editor/advanced-config-editor/#/en/editor |
37f6586
to
a4997ba
Compare
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.
Looks good! We should create a new issue to update the schema to support some of the newer features that have been merged in recently.
Reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yileifeng)
src/components/editor/helpers/custom-editor.vue
line 3 at r1 (raw file):
<template> <div class="mt-4"> <json-editor
Is it possible to make the height of the JSON editor slightly taller? It's quite small when I try to use it on my laptop, and there's still some space to show more
a4997ba
to
660229b
Compare
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.
Made a new issue to track future schema changes and added a few minor adjustments for current validation errors as discussed
Reviewable status: 11 of 13 files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA)
src/components/editor/helpers/custom-editor.vue
line 3 at r1 (raw file):
Previously, RyanCoulsonCA (Ryan Coulson) wrote…
Is it possible to make the height of the JSON editor slightly taller? It's quite small when I try to use it on my laptop, and there's still some space to show more
Donethanks
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.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yileifeng)
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.
Reviewed 11 of 13 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yileifeng)
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.
Looks slick!
One minor grouse I had was the advanced editor save button that asks the user for confirmation before overwriting is way down at the bottom and a user may not see it.
Instead, they may click the regular top right save button, which does not ask for any confirmation and proceeds to save. Not a blocking comment since this is not anything breaking.
Reviewed 11 of 13 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yileifeng)
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.
Reviewed 11 of 13 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yileifeng)
d2283ea
to
acc4e05
Compare
acc4e05
to
62cab97
Compare
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.
Removed the confirmation step when saving JSON
Reviewable status: 4 of 14 files reviewed, all discussions resolved (waiting on @KashishMistry, @mohsin-r, @RyanCoulsonCA, and @szczz)
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.
Reviewed 3 of 13 files at r1, 1 of 2 files at r2, 10 of 10 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yileifeng)
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.
Whoops, realized I may have been holding up this PR.
Looks ALL GOOD!
Reviewed 10 of 10 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yileifeng)
Related Item(s)
Closes #190
Related to #247
Changes
Testing (in progress)
Any testing involving the custom ("advanced") editor by modifying/adding new config properties for slides and attempting to save. As an example, adding "customStyles": "text-align: center;" to a text panel should apply changes.
If the schema validation error alert is encountered upon saving, it is possible the schema requires more adjustments for missing/outdated properties (NOTE: likely to fail if using custom editor to add niche chart properties or RAMP configuration since those are not contained in panel schema, but in these cases the user should be using the highcharts editor or RAMP config editor respectively).
This change is