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 schema to be created with JSON config in UI #11809

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

jayeshchoudhary
Copy link
Contributor

@jayeshchoudhary jayeshchoudhary commented Oct 15, 2023

this is ui, feature

Closes - #11789

Description

  • Add UI to add Schema directly using JSON
  • This is helpful when user has the entire JSON handy
  • UI still keeps Form Way to add Schema along with JSON view

Screenshot

schema-add-json.mp4

@jayeshchoudhary
Copy link
Contributor Author

Please review - @ksnijjer @Jackie-Jiang

@codecov-commenter
Copy link

codecov-commenter commented Oct 15, 2023

Codecov Report

Merging #11809 (612235f) into master (cedac51) will increase coverage by 0.13%.
Report is 10 commits behind head on master.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master   #11809      +/-   ##
============================================
+ Coverage     63.10%   63.24%   +0.13%     
+ Complexity     1140     1139       -1     
============================================
  Files          2343     2343              
  Lines        126306   126343      +37     
  Branches      19419    19439      +20     
============================================
+ Hits          79703    79903     +200     
+ Misses        40932    40758     -174     
- Partials       5671     5682      +11     
Flag Coverage Δ
custom-integration1 <0.01% <ø> (?)
integration <0.01% <ø> (-0.01%) ⬇️
integration1 <0.01% <ø> (-0.01%) ⬇️
integration2 0.00% <ø> (ø)
java-11 63.20% <ø> (+0.12%) ⬆️
java-17 63.08% <ø> (+0.11%) ⬆️
java-20 63.05% <ø> (+0.08%) ⬆️
temurin 63.24% <ø> (+0.13%) ⬆️
unittests 63.23% <ø> (+0.13%) ⬆️
unittests1 67.37% <ø> (+0.13%) ⬆️
unittests2 14.45% <ø> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 38 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@xiangfu0 xiangfu0 added feature ui UI related issue labels Oct 15, 2023
@xiangfu0
Copy link
Contributor

This is fantastic!
Thanks @jayeshchoudhary !

Copy link
Contributor

@walterddr walterddr left a comment

Choose a reason for hiding this comment

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

looks good to me.
do you have a demo that edit/paste a valid JSON and switch back to original view (does it shows the line items parsed properly?

@jayeshchoudhary
Copy link
Contributor Author

jayeshchoudhary commented Oct 15, 2023

looks good to me. do you have a demo that edit/paste a valid JSON and switch back to original view (does it shows the line items parsed properly?

No, we reset the config when the view gets changed. So you will see a fresh form when you change to simple view again same with JSON view.
This is because to parse values back and forth we need to check many conditions/validations, plus working with forms make it even more complex.

To make it simple and not introduce any bugs this is good imo.

@xiangfu0 xiangfu0 merged commit 104cc6e into apache:master Oct 16, 2023
19 checks passed
@xiangfu0
Copy link
Contributor

looks good to me. do you have a demo that edit/paste a valid JSON and switch back to original view (does it shows the line items parsed properly?

I think we can add this later.

@walterddr
Copy link
Contributor

i dont really think we need to add later. simply have a banner warning when switching view "you have unsaved schema edit, do you want to proceed" is intuitive enough. and frankly i dont think any user will mix the view usages

i asked the question simply want to make sure ppl arent allow to do exactly that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ui UI related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants