Skip to content
This repository has been archived by the owner on Nov 2, 2023. It is now read-only.

Support short label and guidance #242 #244

Merged
merged 16 commits into from
Jan 29, 2021
Merged

Conversation

florianm
Copy link
Contributor

@florianm florianm commented Dec 11, 2020

Add support for short label and guidance as per #242 and discussions on ODK Slack.

  • control.js
    • add short and guidance to sidebar
    • add context help to sidebar footer
  • styles.css
    • add styles for short and guidance - as a very minimalistic start, kept them similar to label / hint but decreased font size. Arguably could improve placement within widget (line breaks between label and hint?) and styling.
  • index.erb
    • add short and guidance to draggable field widget
  • data.js
    • extend addTranslation to allow a nested object (short nested within label, guidance nested within hint)
    • modify parseControl > label/hint to feed short/guidance to addTranslation if present

A few impressions:
Form builder draggable widget
image

Enketo preview
image

Collect form

  • not sure how to trigger "short" label
  • guidance works as expected

Attached
A minimal form with one text field with short and guidance, and one without, as shown in screenshots as .odkbuild and .xml and exported .xlsx. The xslx discards short and guidance but shows that this PR will at least not break build2xlsform.
Short guidance.zip

Related


This change is Reviewable

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.

Agree the styling is not delightful but I think it's fine for now.

The short label concept is to provide condensed text that can be shown in a summary or table of contents view. So it looks like Enketo doesn't really support it as intended. To see a short label in Collect, go to the "jump" view.

https://github.com/getodk/build/pull/244/files#diff-ba8409fc6425bc4d7fb9b5152c790bd614d0c382f4f3463c4dbab6031eba5250R212 looks a little convoluted to me but I don't know the context well enough to be able to assess. Would be great to get a quick look from @issa-tseng

public/javascripts/control.js Outdated Show resolved Hide resolved
@florianm
Copy link
Contributor Author

florianm commented Dec 11, 2020

re styling: Edit: added some line breaks and styling to improve readability, see below. Absolutely happy to follow directions by core team on layout and styling though.

re addTranlation:
Context: Short and Guidance require Build to place their displayed translations as a sibling value nodes to the translations for Label and Hint, respectively. To achieve this duplication of value nodes,
https://github.com/getodk/build/pull/244/files#diff-ba8409fc6425bc4d7fb9b5152c790bd614d0c382f4f3463c4dbab6031eba5250R212 retrieves the translations for short and guidance in parallel to the main translation (e.g. label and hint). The simplest and most robust approach was to duplicate the while loop. This allowed me to remain in the same scope and simply add obj1 and result2. Consider that the bare-bones MVP.
Suggestions from my "later basket":
Extracting the repeating while loops into a separate function might lend itself to write unit tests. Adding some comprehensive docstrings would make the code more accessible and readable to new contributors. (I deliberately kept to the original inline comment style.)
Interested to hear what @issa-tseng feels appropriate / in scope / out of scope here.

@florianm
Copy link
Contributor Author

florianm commented Dec 13, 2020

Here's an alternative layout and styling that feels slightly more readable:
image
image

  • Short label gets a slightly brighter background (#888) than the surrounding bg (`#777~), margin left provides spacing to label, padding and round edges size the lighter background to make it look like a twitter-bootstrap label.
  • Short collapses entirely if :empty.
  • Hint becomes a h5 to go on a new line
  • Guidance becomes a h6 to become a new line
  • Name gets a 0.5em margin-top to separate visually from hint and guidance
  • Short, hint, and guidance are hidden in collapsed view
  • Control doesn't get vertically larger, hint and guidance newlines utilize free space inside the control

If this were my own code I'd add comments to the respective CSS rules to explain the above, but I didn't want to pollute the otherwise comment-free CSS.
I did remove the // #242 comments referring to this PR from the comments and slightly reworded them.

@florianm
Copy link
Contributor Author

florianm commented Dec 14, 2020

67c0e5c adds support for media labels and refactors handling of extra control objects in addTranslation considerably. This clarifies the clutter and allows to handle 0..* extra control elements instead of exactly one extra (short or guidance).

I'll test locally within the next day and am open to any suggestions and directions.

Attached: Form with translations (En and Bv) as odkbuild and XML. XML also below.
Short guidance.zip

<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:jr="http://openrosa.org/javarosa" xmlns:ev="http://www.w3.org/2001/xml-events" xmlns:odk="http://www.opendatakit.org/xforms">
  <h:head>
    <h:title>Short guidance</h:title>
    <model>
      <instance>
        <data id="Short-guidance" version="1607927578">
          <meta>
            <instanceID/>
          </meta>
          <question1/>
          <question2/>
        </data>
      </instance>
      <itext>
        <translation lang="English">
          <text id="/data/question1:label">
            <value>Long form label</value>
            <value form="short">Short label</value>
            <value form="image">jr://media/image.png</value>
            <value form="video">jr://media/video.avi</value>
            <value form="audio">jr://media/audio.mp3</value>
            <value form="big-image">jr://media/big_image.jpg</value>
          </text>
          <text id="/data/question1:hint">
            <value>Hint text</value>
            <value form="guidance">Guidance text</value>
          </text>
          <text id="/data/question1:constraintMsg">
            <value></value>
          </text>
          <text id="/data/question2:label">
            <value>Long label but no short label</value>
            <value form="short"></value>
          </text>
          <text id="/data/question2:hint">
            <value>Hint but no guidance</value>
            <value form="guidance"></value>
          </text>
          <text id="/data/question2:constraintMsg">
            <value></value>
          </text>
        </translation>
        <translation lang="Bavarian">
          <text id="/data/question1:label">
            <value>Wos du wuist, Biascherl</value>
            <value form="short">Hoits Mei und sogs kiaza</value>
            <value form="image"></value>
            <value form="video"></value>
            <value form="audio"></value>
            <value form="big-image"></value>
          </text>
          <text id="/data/question1:hint">
            <value>Wannst a weng mehra sogn megst</value>
            <value form="guidance">Wannst no mehra sogn megst</value>
          </text>
          <text id="/data/question1:constraintMsg">
            <value></value>
          </text>
          <text id="/data/question2:label">
            <value>Nur a lange Beschriftung</value>
            <value form="short"></value>
          </text>
          <text id="/data/question2:hint">
            <value>A Ratschlag</value>
            <value form="guidance"></value>
          </text>
          <text id="/data/question2:constraintMsg">
            <value></value>
          </text>
        </translation>
      </itext>
      <bind nodeset="/data/meta/instanceID" type="string" readonly="true()" jr:preload="uid"/>
      <bind nodeset="/data/question1" type="string" jr:constraintMsg="jr:itext('/data/question1:constraintMsg')"/>
      <bind nodeset="/data/question2" type="string" jr:constraintMsg="jr:itext('/data/question2:constraintMsg')"/>
    </model>
  </h:head>
  <h:body>
    <input ref="/data/question1">
      <label ref="jr:itext('/data/question1:label')"/>
      <hint ref="jr:itext('/data/question1:hint')"/>
    </input>
    <input ref="/data/question2">
      <label ref="jr:itext('/data/question2:label')"/>
      <hint ref="jr:itext('/data/question2:hint')"/>
    </input>
  </h:body>
</h:html>

@lognaturel
Copy link
Member

lognaturel commented Dec 14, 2020

Thanks for continuing to improve this! Looks great to me. Upon further reflection, I probably would have omitted short labels and guidance text from the inline question box but I don't have strong feelings about it.

I think it would be helpful to make it clear that label and hint are the two most common text elements used and that the others are optional and less commonly used. In fact, I think they're less common than any of the other options available in the side bar now. Could we put the new options at the bottom? That is, keep everything as it exists now and after the "Invalid Text" UI element, then put all the new hints and labels. I think it would be good to get a sanity check from @issa-tseng before you implement any change, though.

@florianm
Copy link
Contributor Author

florianm commented Dec 14, 2020

Re-ordering $.fn.odkControl.defaultProperties to sort by priority would solve the UX problem that the new additions crowd out the other fields and could confuse existing Build users. Ordered dicts be thanked, that's a two second change.

This is how @lognaturel's suggested order would look:
image
image

@lognaturel
Copy link
Member

I'd move the length constraint above short label as well but I agree that's not half bad! As I said in Slack, I was leaning towards a single generic media label but I think this works. I would not require any jr:// prefix on media, though. I think folks should put in their filename and given that each media type has its own field Build can add the appropriate prefix.

Maybe the media types could have a (filename) hint? e.g. "Audio label (filename)"

@florianm
Copy link
Contributor Author

The current preview places the new fields after the shared properties. (It's also the lazy implementation that just shuffles them to the bottom of $.fn.odkControl.defaultProperties.)
Happy to attempt further shuffling, let me know where the best place would be.

Media field labels - including a (filename) makes perfect sense. Even better would be an indication whether a plain filename would do, or a path is needed, and how the path relates to the contents of the media folder. The field description or tips would be a good place for a working example.

Media file prefix and paths - I need to read up and experiment more with media file paths to understand what's a valid path and how the folder structure inside the form media should look like. Any pointers would speed up my process but I'll get there eventually.

@lognaturel
Copy link
Member

lognaturel commented Dec 14, 2020

how the folder structure

It's all flat. Technically a server or client could segregate media by type but in practice I don't know of any that do. Regardless, that will be up to the user and the server to negotiate once the form gets used. So a user really just needs to provide a full filename with extension and you just need to prepend the correct connector based on media type (though in practice it wouldn't really matter if they were all the same).

* Media label controls could alternatively be collapsed like Advanced
* Likert appearance for SelectOne but not SelectMultiple
@florianm
Copy link
Contributor Author

Pushed a4878cf to address

…help text paths

* Prevent empty itext nodes
* Only media labels are now prefixed with jr://[images, video, audio]/
* Help text paths changed from /xforms/data/path to /data/path/field
@florianm
Copy link
Contributor Author

Up to now, this PR does everything the way @lognaturel described and I understood. I'll need a day (Wed) to test the produced XML and verify that everything works as intended, then this PR will be ready for review.

Florian Mayer added 2 commits December 16, 2020 10:53
* Resize properties pane from 27 to 35em
* Shrink workspace accordingly
* Fix layout of advanced properties to show all fields (calculate was hidden) and prevent unsightly overlap
@florianm
Copy link
Contributor Author

florianm commented Dec 16, 2020

Short guidance media likert.zip

Here's my test form including media files and a quick write-up.

Test devices:

  • Vivo model 1937 smartphone with Vivo's Android 10 distro FuntouchOS
  • Samsung Tab S2 (2018) tablet, Android 7.0
  • ODK Collect 1.29.0 Beta2 on each
  • Using attached test form -
  • Media file name helptext vs jr://[images|video|audio]/ prefix in exported XML seems to be correct - thanks for your time and patience explaining that to me, @lognaturel!
  • Short label and guidance work independently of other fields, i.e. they don't seem to interfere or break things.
  • Likert appearance for select1 works.
  • Image label works for text, numeric, select1, select*. This PR shows controls for all four types of media labels for all other input types too, but I haven't added them yet to the test form.
  • Audio label works across all tested inputs (text, numeric, select1, select*).
  • BigImage label (bigimage.png in combination with a smaller image.png) works with select1 and select*, but does not work with text or numeric inputs.
  • BigImage works with all tested input types (including text and numeric) in Enketo. Possible bug in Collect?
  • Video label (video.mp4) works with select1 and select*, but does not work with text or numeric inputs.
  • Video works with all tested input types (including text and numeric) in Enketo. Possible bug in Collect?
  • Collect curiosity that's gonna trip my users: getting out of a bigimage requires swiping down on my smartphone (no back button shown), but has back button shown on Samsung tablet, getting out of a video label requires tap and back button.

a26d629 is gonna be controversial, as it changes the sacred sidebar styling to make it 35em wide. While I hope not to upset anyone with strong feelings about the sidebar width and am happy to revert this change, this fix improves readability, prevents overflow, and makes sure the calculate field is still visible.

This PR will close #242, #172 and #173.

Happy to hand over this PR to @issa-tseng or anyone else for review, and looking forward to any and all review suggestions.

@issa-tseng
Copy link
Member

issa-tseng commented Dec 17, 2020

i will admit to having not read or run the code, and not done a very close reading of this thread. but here are some thoughts:

  • i think making the sidebar wider is a good call.
  • i don't know that showing the short label in the question box is that useful because it's not really self-evident to me what it is. everything else we put in that box sort of describes what it is by its own nature ("read-only", "required", etc).
  • tbh my first instinct would have been to simply show the short label instead of the full question text whenever it is provided.
    • possibly w a (…) icon at the end that would produce the full question text on hover, or
    • possibly w the full question text in smaller print on its own line below the headline, where the short label now lives.
  • that is a lot of new fields. do we really need them all, and especially can we put some in eg advanced? if someone has eg 4-5 languages going on that's going to be a lot of scrolling to find the stuff you used to use.
  • in general the field ordering in the sidebar is not the easiest to change. if the built in methods to do so are not sufficient, i would advise we just put up with it. that all was written 12 years ago now with no thought for the year 2020.

Florian Mayer added 2 commits December 18, 2020 15:12
* Update links to getodk/build GH repo
* Update author names
* Swap short and long label positions
@florianm
Copy link
Contributor Author

Thanks for the feedback @issa-tseng.

Re short label in question box
I really like always seeing the long form label (i.e. non-collapsed / hover) as it is prominently shown in Collect.

Latest commit as suggestion:
image
Happy to implement any other preferred option too.

Re lots of new fields
Appended all new fields to advanced (below calculate), which now magically just worked (it didn't prior to my CSS changes to advanced area).
I only need the image label for myself, but anticipated future requests for all other possible fields.

Re changes to code
The only non-trivial code changes I made were to refactor addTranslations and extend its args to include a dict of all the extra fields. I've also added docstrings to the new methods for clarity and ease of review/maintenance. I'd love to add unit tests for the new JS bits but will leave the decision on adding a test suite/runner to the core team.

Finally found a few outdated links and names in the index.erb, hope you don't mind me updating them. Didn't add new places of implementation (Thevenard Island and Perth) - remember, only if it contains turtles it's a real PR from Florian, else it's just a sparkling commit.

@issa-tseng
Copy link
Member

how about you drop the text size of the long label then? what does that look like?

@florianm
Copy link
Contributor Author

The above screenshot is 1.8em. Here's 1.2em:
image

@issa-tseng
Copy link
Member

no, the long label.

@florianm
Copy link
Contributor Author

florianm commented Dec 21, 2020

Ah sorry, I was committing before coffee. Should the long label be smaller than the short one?
Currently, the short label is 1.8em (ignoring my undercaffeinated last commit), long label is 2em.

Before I commit the style change:
How do you like short 1.8 and long 1.4?
image

With a sprinkle of margin-top for hints:
image

@issa-tseng
Copy link
Member

can you make the long label small only if there is a short label? then i think i'm good w it. otherwise it's fine as is i think? what do you reckon?

@florianm
Copy link
Contributor Author

florianm commented Dec 21, 2020

Conditional text resizing sounds like a neat UX feature. I've pushed the static sizes as is. Feel free to merge if the status quo is good enough.

I think to format label dependent on presence of short label, we'd have to modify https://github.com/getodk/build/blob/master/public/javascripts/control.js#L81-L99 - does that sound right? I can address this in about 12h from now.

@issa-tseng
Copy link
Member

depends if you're applying a class based on whether short exists or not.

in most browsers something like

.short + .long { font-size: a-bit-smaller; }
.short:empty + .long { font-size: a-bit-bigger; }

should work

@florianm
Copy link
Contributor Author

Thanks for the pointer, I found the incantation. This is how the latest commit looks:
image

Does 1.4em for a long label after a short label and 2em for only a long label look right?

@issa-tseng
Copy link
Member

i'm p cool w that. @lognaturel ?

@lognaturel
Copy link
Member

Works for me! Thanks for that good iteration!!

@lognaturel lognaturel merged commit a26c17a into getodk:master Jan 29, 2021
@florianm florianm deleted the 242-guidance branch January 9, 2022 05:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants