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

allow editing in json editor for checked params #787

Merged
merged 18 commits into from
May 5, 2021
Merged

Conversation

ThatGalNatalie
Copy link
Contributor

@ThatGalNatalie ThatGalNatalie commented Apr 22, 2021

PR Checklist:

  • add a short description of what's changed to the top of the CHANGELOG.md
  • add/update tests (or don't, for reasons explained below)

Describe this PR

The JSON editor wasn't reflecting the checked/unchecked parameters. When modifying an existing schedule, the checked parameters should now be the only parameters that is displayed

@ThatGalNatalie
Copy link
Contributor Author

@dylanbhughes

@ThatGalNatalie ThatGalNatalie marked this pull request as ready for review April 22, 2021 18:49
@dylanbhughes
Copy link
Contributor

Having trouble looking in Netlify but the code makes sense to me!

@zhen0
Copy link
Member

zhen0 commented Apr 23, 2021

This addresses the issue it was asked to nicely but I fear that the JSON edit experience could still lead to mistakes. If I go to the schedule parameters tab and immediately switch into JSON, add my parameters and then switch back to set the schedule, I'd miss that I need to check the box to include my parameters. Could we add a JSON edit specific warning?

Copy link
Member

@zhen0 zhen0 left a comment

Choose a reason for hiding this comment

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

I think this fixes the issue @ThatGalNatalie - thanks. However, I think we risk wiping all a user's JSON input. Can we add a warning that they also need to check the boxes? Or remove the need to check boxes?

@znicholasbrown
Copy link
Contributor

Previews enabled for this PR 👍

@ThatGalNatalie
Copy link
Contributor Author

update:
Latest commit should allow you to edit in the JSON editor and the parameters should be checked/unchecked based on that.

However, if you check a parameter then it will be added to the jsoninput but if you decide to uncheck a parameter it will not be reflected in the json editor. I think it has something to do with the this.value computed prop and how we’re checking what gets returned but can’t seem to wrap my head around it and need some help on that.

Tested the run page briefly and it seems to not be affected by this change but will need to do a more thorough test

Copy link
Contributor

@znicholasbrown znicholasbrown left a comment

Choose a reason for hiding this comment

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

2 suggestions which I think should fix the issue you're seeing

src/components/CustomInputs/DictInput.vue Outdated Show resolved Hide resolved
src/components/CustomInputs/DictInput.vue Outdated Show resolved Hide resolved
Copy link
Member

@zhen0 zhen0 left a comment

Choose a reason for hiding this comment

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

One small suggestion

src/components/CustomInputs/DictInput.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@znicholasbrown znicholasbrown left a comment

Choose a reason for hiding this comment

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

This LGTM @ThatGalNatalie 👍

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.

4 participants