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

Feature: Enhancements to run page run-config inputs with new dict, json, yaml inputs #1095

Merged
merged 89 commits into from
Nov 11, 2021

Conversation

stackoverfloweth
Copy link
Contributor

@stackoverfloweth stackoverfloweth commented Oct 25, 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

Built new "code input" components to replace DictInput, JsonInput, and YamlInput. Also created ResettableWrapper, broke out separate forms for each run config type, and other minor code/ux improvements

@stackoverfloweth
Copy link
Contributor Author

If I have a type of Date and I switch to the json tab and then back to the dict tab, my input gets cleared
Screen.Recording.2021-10-28.at.12.01.48.PM.mov

I think this issue is resolved but I'm seeing a few oddities between reset and type, most clearly when I use the date select (which is really cool aside from that!):

Screen.Recording.2021-11-03.at.12.15.50.PM.mov
Also wonder if we should set the default date to be today? Actually it is - the 1969 date is an oddity because of existing integer value.

great find @zhen0, I was able to fix that without a significant refactor but it still might be worth running through your QA once more to validate that I didn't break something else?

@znicholasbrown
Copy link
Contributor

I've taken a look at this but want to do another pass with the changes requested by @zhen0 & @ThatGalNatalie - will submit a review this evening

@ThatGalNatalie
Copy link
Contributor

I just noticed this while looking at #1104 but the screen freezes for me when I switch from list to date with a value present

freeze.mov

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.

Just a few notes wrt error states @stackoverfloweth:

  • With validation on most of the dict-type input fields, focusing and bluring the inputs without any changes forces them into an error state
  • Selecting an "Auto" type explicitly after typing in a key/value forces the component into an error state
  • When part of the form is in an error state, the form shouldn't be submittable

Otherwise these changes look great!

@stackoverfloweth
Copy link
Contributor Author

stackoverfloweth commented Nov 8, 2021

Just a few notes wrt error states @stackoverfloweth:

  • With validation on most of the dict-type input fields, focusing and bluring the inputs without any changes forces them into an error state
  • Selecting an "Auto" type explicitly after typing in a key/value forces the component into an error state
  • When part of the form is in an error state, the form shouldn't be submittable

Otherwise these changes look great!

@znicholasbrown requested changes have been implemented, thanks for the suggestion!

@stackoverfloweth stackoverfloweth dismissed znicholasbrown’s stale review November 8, 2021 13:48

suggestions have been addressed

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.

Tested with local run, universal run and K8s run - all go through with no (or expected) issues. Switching types also no longer causes any problems.

I'd like to wait to merge this to staging until we do a release (tomorrow) so that we can demo etc before we release these changes to prod.

znicholasbrown
znicholasbrown previously approved these changes Nov 9, 2021
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.

LGTM! (there are changelog conflicts but I think those will be resolved elsewhere 👍🏻 )

@zhen0 zhen0 changed the title Run-config-forms Feature: Enhancements to run page run-config inputs with new dict, json, yaml inputs Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants