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

Cherrypicked improvements and corrections from https://dimagi.github.io/xform-spec/ #53

Merged
merged 11 commits into from
Dec 23, 2016

Conversation

MartijnR
Copy link
Contributor

@MartijnR MartijnR commented Dec 12, 2016

#33

Dimagi developers have reviewed and published their spec, so I just had to figure out which parts of their XForms spec are relevant to our XForms spec.

There are various outstanding issues, but they are not related to this commcare-merge and are covered under separate issues and PRs (e.g. external applications and preload items).

This PR brings the spec up to a fair level from which we can target the remaining more specific issues in separate PRs.

@MartijnR
Copy link
Contributor Author

MartijnR commented Dec 13, 2016

Please somebody kindly review this. XForm specifications are so much fun! 🎉

All other issues will have to stacked on top of this CommCare merge to avoid conflicts.

@MartijnR
Copy link
Contributor Author

@yanokwa Do you know anybody to review this perhaps? Otherwise, I'm totally fine merging myself. :shipit:

@lognaturel
Copy link
Member

Oops I wasn't following this repo when your PR came in. I'm not a super expert but I'll do a pass through and let you know what I see!

@MartijnR
Copy link
Contributor Author

Thanks. You're going to love it! :)

Copy link
Member

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

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

So fun! Apologies for the delay, I had some ramping up to do. The changes look great. In my review I focused on the changes and making sure they were consistent with the code at https://github.com/opendatakit/javarosa/ and made sense. Mostly I made some typo suggestions and had a few questions about things that could be added.

I wonder whether now that the project is becoming more community-owned it would be helpful to have an extra paragraph at the beginning providing context on who this spec is for and how it should be used. Could be a separate PR.

@@ -27,18 +26,19 @@ Below is an example of a complete and valid ODK XForm:
<h:title>My Survey</h:title>
<model>
<instance>
<data id="mysurvey" version="2014083101">
<data id="mysurvey" orx:version="2014083101">
Copy link
Member

Choose a reason for hiding this comment

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

I know the document pre-supposes a good working understanding of XML but I'm wondering whether it might be worth linking to a quick resource about namespaces along with introducing this (good) change? It might also be good to explain things like the value of having separate OpenRosa XForms and OpenRosa javarosa namespaces as defined in this example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's excellent. I've added a separate issue for this, so this PR stays focused on #33.

| `id` | on the childnode of the primary instance: This is the unique ID at which the form is identified the server that publishes the Form and receives data submissions. For more information see [this Form List Specification](https://bitbucket.org/javarosa/javarosa/wiki/FormListAPI).
| `version` | on the childnode of the primary instance can contain any string value.
| `jr:template` | on any repeat group node: This serves to define a default template for repeats and is useful if any of the leaf nodes inside a repeat contains a default value. It is not transmitted in the record. For more details, see the [repeats](#repeats) section.
| `id` | on the childnode of the primary instance: This is the unique ID at which the form is identified the server that publishes the Form and receives data submissions. For more information see [this Form List Specification](https://bitbucket.org/javarosa/javarosa/wiki/FormListAPI). \[required\]
Copy link
Member

Choose a reason for hiding this comment

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

This sentence doesn't parse. Maybe "on the server that publishes..."



### Secondary Instances
### Secondary Instances - Internal
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest holding off on adding "Internal" until there's a corresponding "External" (#50). Or maybe acknowledging that there's a proposal in place and linking to it could be interesting.

@@ -4,6 +4,8 @@ title: Bindings

A `<bind>` element wires together a primary instance node and the presentation of the corresponding question to the user. It is used to describe the datatype and various kinds of logic related to the data. A bind can refer to any node in the primary instance including repeated nodes_. It may or may not have a corresponding presentation node in the [body](#body).
Copy link
Member

Choose a reason for hiding this comment

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

I think the _ after "nodes_" is a typo.

| `uuid()` | Return a random [RFC 4122 version 4](http://tools.ietf.org/html/rfc4122) compliant UUID string [review]()
| `log(number arg)` | As in [XPath 3.0](http://www.w3.org/TR/xpath-functions-30/#func-math-log).
| `log10(number arg)` | As in [XPath 3.0](http://www.w3.org/TR/xpath-functions-30/#func-math-log10).
| `today()` | Returns today's date without a time component.
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to explicitly call out the format and/or say it's DateUtils.roundDate(new Date()), same with now() below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be good to explicitly call out the format

Yes, it would be. I've added a separate issue: #56

| `<item>` | Child of `<select>` or `<select1>` that defines an choice option.
| `<itemset>` | Child of `<select>` or `<select1>` that defines a list of choice options to be obtained elsewhere (from a [secondary instance](#secondary-instances)).
| `<value>` | Child of `<item>` or `<itemset>` that defines a choice value.

Below is an example of a labels, an output, a hint, an itemset and value used together to define a form control:
Copy link
Member

Choose a reason for hiding this comment

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

Should say "a label" rather than "a labels".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excessive amount of "s" (due to cmd-S shortcut) ;)


### Appearances

The appearance of the 5 form controls can be changed with the appearance attributes. Appearance values usually relate to a specific [data type](#data-types). See the [XLS Form specification](http://xlsform.org) for a list of appearance attributes are available for each data type. Multiple space-separated appearance values can be added to a form control
The appearance of the 5 form controls can be changed with appearance attributes. Appearance values usually relate to a specific [data](#data-types) or [question](#body-elements) type. See the [XLS Form specification](http://xlsform.org) for a list of appearance attributes are available for each data type. Multiple space-separated appearance values can be added to a form control in any order.
Copy link
Member

Choose a reason for hiding this comment

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

Missing "that" or extra "are" in "attributes are available"

@@ -43,7 +45,7 @@ A `<repeat>` uses the nodeset attribute to identify which instance node (and its

### Creation, Removal of Repeats

The default behaviour of repeats is to let the user create or remove repeats using the the user interface. ODK Collect will ask for the first repeat. Enketo will show the first repeat automatically. This can be disabled by adding the attribute `jr:noAddRemove="true()"` to the `<repeat>` element.
The default behaviour of repeats is to let the user create or remove repeats using the the user interface. The user control for creating and removing repeats can be disabled by adding the attribute `jr:noAddRemove="true()"` to the `<repeat>` element.
Copy link
Member

Choose a reason for hiding this comment

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

Should probably say "behavior" as the rest of the doc uses American spelling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. I still sometimes regress toward using proper English...

@@ -1,9 +0,0 @@
---
title: Future
Copy link
Member

Choose a reason for hiding this comment

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

Maybe replace this with a link to the issue tracker and an invitation to join the conversation?

@@ -1,8 +0,0 @@
---
title: Apps
Copy link
Member

Choose a reason for hiding this comment

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

Why get rid of this?

Copy link
Contributor Author

@MartijnR MartijnR Dec 22, 2016

Choose a reason for hiding this comment

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

It didn't seem right to me. I'm afraid "we" end up having to assess whether an app is compliant enough to make it to this list (and then keep doing that while the app evolves). I think it's better for tools themselves (such as Enketo) to advertise that they are following the spec. There is also the question of whether proprietary tools should be added and if not, what are acceptable licenses.

@MartijnR
Copy link
Contributor Author

Thanks a lot! I'll go through your comments with a quickness!

@MartijnR
Copy link
Contributor Author

I think that takes care of everything (or moved it to other issues).

Will do a few more PRs this month and one of them to get rid of that ugly banner at the top!

@lognaturel lognaturel merged commit 24993a1 into gh-pages Dec 23, 2016
@lognaturel lognaturel deleted the cherrypick-merge-commcare-1-squashed branch December 23, 2016 16:02
@lognaturel
Copy link
Member

Sounds great!

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