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

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

hxh-robb
Copy link

@hxh-robb hxh-robb commented Jan 15, 2018

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

Copy link
Author

@hxh-robb hxh-robb left a comment

Choose a reason for hiding this comment

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

New feature "appendedProperties" for issue #22

The following link is the demo of this feature
https://jsfiddle.net/hxh_robb/xk663z01/

@hxh-robb hxh-robb changed the title schema.enum enhancement for issue #85 schema.enum enhancement for issue #85 and #22 Jan 18, 2018
@juamar
Copy link

juamar commented Mar 5, 2018

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
Copy link
Author

hxh-robb commented Mar 6, 2018

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
Copy link

juamar commented Mar 6, 2018

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
Copy link

juamar commented Mar 6, 2018

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
Copy link
Author

hxh-robb commented Mar 7, 2018

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
Copy link

juamar commented Mar 7, 2018

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?

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.

2 participants