search

brutusin / json-forms

JSON Schema to HTML form generator, supporting dynamic subschemas (on the fly resolution). Extensible and customizable library with zero dependencies. Bootstrap add-ons provided
http://brutusin.org/json-forms
Apache License 2.0
606 stars 168 forks source link

schema.enum enhancement for issue #85 and #22 #114

Open hxh-robb opened 6 years ago

hxh-robb commented 6 years ago

Use decorator to meet this requirement is not intuitive, so I made enum rendering accept a {text:XXX, value:XXX} object to render display text and value respectively

juamar commented 6 years ago

Ohh, I have made this in my branch because of some projects needs i am working on! (not in the pull request initialDate) #113. Maybe we can merge our work... My implementation seems much more simpler than yours at first glance. I have not commit my change yet to github because i have not tested it as much as i will like to, but as far as i tested it, it works. My solution is supposed to work the way the original brutusin works (just an array within enum) and ours, depending on the schema (Key,Value). Does your improvement work with the bootstrap-select Brutusin let us to enchance? (http://silviomoreto.github.io/bootstrap-select/examples/) I think it is not possible.

hxh-robb commented 6 years ago

Hi juamar, can you post a demo of yours, sounds good that yours is simpler, which means it would be more easier to use.

The reason I made a new schema option appendedProperties is just because I don't want to change the original rendering logic too much(Following the Open-Closed Principle). With the new option, I can work with it in my own renderAppendedProperties function without touch the original one, so that if something go wrong in my code it won't hurt the original function.

And you're right, my code doesn't work with the bootstrap-select. If there is a multiple select field, things might go wrong, but I'm not sure, I'll test it later. BTW does your version finish this work? it would be great to see this enhancement.

juamar commented 6 years ago

At night (Madrid Time) i will try to test it a little bit more with a JSFiddle and commit my update in any case.

juamar commented 6 years ago

All done. My fork: https://github.com/juamar/json-forms/tree/enumKeyValue Some testing: http://plainsite.tecandweb.net/brutusin/ (I wasn't able to make fiddle to work since i don't have some CDN at hand nor a testing site with HTTPS).

You are right, i am not taking in account the open-closed principle because my editions are within the original implementation. I suppose further testing will be good in my code, but i am confident my new code will not compromised other functionality from brutusin. Muy changes affects only the enum rendering block.

Meanwhile testing, I saw that I was using JQuery to select the option from value. Now I don't.

Just in case, in the example, the form on the left uses a Dictionary (key-value) as enum, and the one on the right uses an array (as the original implementation, showing that both ways are compatible in this new code).

hxh-robb commented 6 years ago

Hi juamar, I've had a look at your demo, seems that yours is much more clear and concise. I'll check out your brunch and see how to merge it into mine.

BTW, you didn't implement the equivalent appendedProperties feature in your fork, right?

juamar commented 6 years ago

No, i don't... But we got a situation here... I think we are not taking in account this: https://github.com/brutusin/json-forms/blob/master/README.md#status

I mean if @idelvall will not take care of this repo any more, who knows when our pull request will be taken in consideration.

I am not interested in appendedProperties functionality right now, so, i will correct my self, i am only solving issue #85 . Maybe you can add it. The question is... Where to make the pull request? Where to continue this project?