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

Use bracket notation for paths #43

Merged
merged 9 commits into from
Aug 14, 2014
Merged

Use bracket notation for paths #43

merged 9 commits into from
Aug 14, 2014

Conversation

mike-marcacci
Copy link
Contributor

I switched to using basic bracket notation wherever I saw the path being build. I also added a function to convert dot notation into bracket notation which preserves unicode characters, any embedded quotes, dots and brackets.

However, that function seems to be getting passed the primary (first-level) property unescaped, so the presence of brackets in the key name (which should also be allowed) breaks it.

Right now this fixes #41

@davidlgj
Copy link
Contributor

Since this breaks the tests I'll do a manual merge when I get the time. Looks like it's pretty straightforward with the tests though, mostly asserts that depend on dot notation.

@mike-marcacci
Copy link
Contributor Author

Hi david – I'd actually recommend holding off... I'm working on a fix that allows any character to be used in the key name including "["

My new (much cleaner) solution is to build the path as an array instead of a string, which can then simply be escaped and joined before being injected into the DOM.

@mike-marcacci
Copy link
Contributor Author

OK, so this is not yet ready. I need to:

I just wanted to run this by you conceptually. Check out the "Complex Keys" example. The angular bug effects objects inside objects (fieldsets) which you can see in both the "Complex Keys" example and "Kitchen Sink" tab "more stuff." So this can't go out yet.

Let me know what you think!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.19%) when pulling d5126dc on mike-marcacci:bracket_notation into edec694 on Textalk:master.

@mike-marcacci
Copy link
Contributor Author

OK, so this was a pretty involved update. Due to the Angular bug listed above, I had to upgrade everything to work with Angular 1.3. This included turning off the now-deprecated replace: true flag on decorator directives and updating the corresponding tests.

I also includes the suggested fix for #49 which was a blocker to using 1.3

@davidlgj
Copy link
Contributor

Update: ok you updated and commented while I was writing!

Hi @mike-marcacci,

sorry it took so long to answer, I'm on vacation and don't have that much computer time. Conceptually I think it looks very good. I have no problems with "form keys" being arrays instead of strings internally.

Looks really cool, being Swedish myself I can see a reason for supporting UTF-8 even in key names, not to mention supporting dash, which is a very common use case.

I tried to test the example, but I think you're in the middle of something since the objectpath bower repo has no provider for objectpath, its just a regular service so the use of ObjectPathProvider in shema-form.js does not work, maybe you haven't pushed the code to github yet?

I have one question though, how broken is it on angular 1.2? As I understand angular/angular.js#8039 it basically doesn't work?

Dropping support for 1.2 could work for us, but we're using 1.2 right now, and I guess a lot of other user might be on 1.2 as well. One solution could be to stay with the "dot" notation if 1.2 is detected even though it's broken for a lot of cases (add a "brokenStringify" method) so current users aren't affected and document what happens if you don't use 1.3. What are your thoughts on that?

I also have an item to add to your todo list, could you base it on the development branch instead of master?

Don't worry about the tests, I'll fix them for you. It's mostly asserts of what the value of the ng-model is (i.e. with dot notation and not bracket).

Nice with a bower repo as well, this is very useful code. Thank you!

@davidlgj
Copy link
Contributor

@mike-marcacci Your too fast for me! :)

@mike-marcacci
Copy link
Contributor Author

Haha :) please don't miss your vacation time on my account! I think your suggestion is an excellent way to support both 1.2 and 1.3. Breaking 1.2 didn't feel right, especially given that 1.3 is still in beta.

I'll rebase from development and should have those changes up soon.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.34%) when pulling 93ba0ed on mike-marcacci:bracket_notation into edec694 on Textalk:master.

@mike-marcacci
Copy link
Contributor Author

OK, signing off for the night, but wanted to let you know that dot notation is restored in Angular 1.2.x environments. I've added a sfPath provider that abstracts the ObjectPath one and applies these rules. When 1.2.x is out of common use that additional layer can be stripped out.

If you open up bootstrap-example.html and swap out Angular versions (I have 1.2.21 in there but commented out), you'll get the previous behavior, and the "Complex Keys" example will be broken.

I'm not exactly how you want to go about testing this, but I'll save that for another day :)

@davidlgj
Copy link
Contributor

Hi @mike-marcacci, just wanted to give you a heads up that I've started testing and hope to merge to development today. Impressive work!

@mike-marcacci
Copy link
Contributor Author

Thanks! Let me know if you find any issues and I'll try to crank them out right away.

@mike-marcacci
Copy link
Contributor Author

@davidlgj, just to throw it out there: if you would rather not add to the dependency list, I'd be OK including the contents of ObjectPath directly in the sfPath provider. Let me know if you'd prefer that, and I can make those changes.

@davidlgj
Copy link
Contributor

@mike-marcacci, I'm fine having it on as a dependency, it's lean and nice and probably useful in other projects. Also it has the added bonus that I don't have to support it ;)

@davidlgj
Copy link
Contributor

btw I'll close the PR when its in master, probably late this week.

@mike-marcacci
Copy link
Contributor Author

Sounds great! Thanks for the crazy responsiveness!

@davidlgj davidlgj merged commit 93ba0ed into json-schema-form:master Aug 14, 2014
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.

3 participants