The repeater! - Githubissues

jneen commented 4 years ago

This pull request includes the repeater, as well as the refactoring necessary to make it work.

Why do we need to refactor like this? The TLDR is: we need to be able to access the data from helpers, which means it needs to be registered on res.locals instead of passed into res.render(...).

lgtm-com[bot] commented 4 years ago

This pull request introduces 11 alerts when merging a2c36cea61befb258ab95ca20ec59ef92c6c8c0b into 6e18ae68fd80332bd9414b35c763b49eaf7e6632 - view on LGTM.com

new alerts:

10 for Unused variable, import, function or class
1 for Useless conditional

jneen commented 4 years ago

Breaking changes include:

rendering is no longer done with res.render(route.name, getViewData(...)). Instead, we use the route.loadKeys(...) or route.loadSchema(Schema) to load data from the session, and the middleware route.render() to render the template. This ensures that all data is available on res.locals and can be seen by helpers. getViewData et al have been removed.
the argument order of radioButtons and checkBoxes have changed, and errors are no longer manually passed in. This makes them consistent with the other controls, and usable inside repeaters. The new order is:

{{ checkBoxes('my_field', 'question.label', { 'val1': 'label1', 'val2': 'label2 }) }}
{{ radioButtons('my_field', 'question.label', { 'val1': 'label1', 'val2': 'label2' }) }}

The checkBoxes control will result in a value that is an object with the selected keys set to the string "on". For example, if I check the box next to 'label1', the backend will receive my_field: { val1: 'on' }. The radioButtons control will result in a string indicating which value was selected - if I select 'label1', the backend will receive my_field: 'val1'.

nunjucks macros now take optional arguments (name=val) for class, autocomplete, and hint, making the API more internally consistent and less verbose.

jneen commented 4 years ago

Because it was easier to include than not, I've also included our radioBlock macro, which I'll add an example for. This is intended for radio buttons that select segments of a form, for example entering height in imperial or metric units:

        {% call radioBlock('heightSystem', null,
                           { 'metric': 'height.enter-centimetres' }) %}
            <div id="metric" style="display: {{displayMetric}};">
                <div class="w-3-4">
                    {{ textInput('cm', 'height.centimetres', { class: 'w-1-8' }) }}
                </div>
            </div>
        {% endcall %}

        {% call radioBlock('heightSystem', null,
                           { 'imperial': 'height.enter-feet-inches' }) %}
            <div id="imperial" style="display: {{displayImperial}};">
                <div class="w-1-8">
                    {{ textInput('feet', 'height.feet', { class: 'w-3-4' }) }}
                    {{ textInput('inches', 'height.inches', { class: 'w-3-4' }) }}
                </div>
            </div>
        {% endcall %}

lgtm-com[bot] commented 4 years ago

This pull request introduces 11 alerts when merging c8e95b0d43179b791c332a0ce497e68a336a6e2c into 6e18ae68fd80332bd9414b35c763b49eaf7e6632 - view on LGTM.com

new alerts:

10 for Unused variable, import, function or class
1 for Useless conditional

jneen commented 4 years ago

@timarney Those are all very valid concerns - I was also a bit worried when I was implementing these. What I eventually came to, though, was that the current API was inconsistent and brittle enough that it was probably best to fix them while we're still in early adoption. After this, we should be able to freely change how things work without breaking user code.

I agree that enterContext and exitContext can be a bit tough to read for those who aren't used to working with dynamic contexts - but the alternative is to place the burden of tracking names, errors, and values on the form designer, which I think is a non-starter, especially since they all have slightly different APIs. I should note that those two functions should only be used by those implementing form controls that use nested content (like the repeater), and never by a form designer, or those implementing simple controls. For those implementing simple controls, getData, getName, and getError are available.

lgtm-com[bot] commented 4 years ago

This pull request introduces 11 alerts when merging 6f5953b48885cf06c863fecca7bbaecc625b57de into 54e682344fa54d9df233f5bc292ed2473bf9cdaf - view on LGTM.com

new alerts:

10 for Unused variable, import, function or class
1 for Useless conditional

timarney commented 4 years ago

@jneen can you ping me when it's a good time to pull down and review?

I currently get an error which assuming is given this is a work in progress.

express-validator: a validator/sanitizer with name default does not exist
Listening on port 3000 production
contextMiddleware
contextMiddleware
☠️ Error => Cannot read property 'msg' of undefined

timarney commented 4 years ago

the argument order of radioButtons and checkBoxes have changed, and errors are no longer manually passed in. This makes them consistent with the other controls, and usable inside repeaters. The new order is:

Given this changes the argument order I would like to take this opportunity to to be able to pass specific attributes to to each radio / checkbox.

I started down that road in a branch called toggle but using the current field order but opting to hold on that work until this is in.

Passing attrs to radios

// note the <input {{inputAttr}} id="{{ key }} ....

<div class="multiple-choice multiple-choice--radios" id="{{ key }}">
                {% if errors and errors[key] %}
                    {{ validationMessage(errors[key].msg, key) }}
                {% endif %}
                {% for index, val in values %}
                    {% set inputAttr = attributes[index] %}
                    <div class="multiple-choice__item">
                        <input {{inputAttr}} id="{{ key }}{{ val }}" name="{{ key }}" type="radio" value="{{ val }}" {% if value == val %} checked="checked" {% endif %} {% if errors and errors[key] %} aria-describedby="{{ key + '-error' }}" aria-invalid="true" {% endif %}>
                        <label for="{{ key }}{{ val }}">{{ __(val) }}</label>
                    </div>
                {% endfor %}
            </div>

View

// note the {% set attrs = 
<div class="notify-type-toggle-container hide">
            {% set attrs = {hint: 'form.send_notifications.desc', yes: "data-toggle=on", no: "data-toggle=off"} %}
            {% set options =  { 'yes':'Yes','no':'No'} %}
            {{ radioButtons('send_notifications',options, data.send_notifications, 'form.send_notifications', errors, attrs, { required: true }) }}
            <div class="toggle">
                {{ checkBoxes('notify_type', {'sms':'SMS','email':'Email'}, data.notify_type, 'form.notify_by', errors, {hint: 'form.notify_by.desc'} ) }}
            </div>
        </div>

With the result being something like this.

This idea being if I need to add "random" attrs to a radio we can do that.

That goes back to this issue when I was thinking of this type of format

Passing attributes to single radios / checkboxes

https://github.com/cds-snc/node-starter-app/pull/42#issuecomment-534096584

{{ radioButtons('group_name_for_input', { 'input_1': {classes: "yyy" , id: '"yyy", data-attr: "yyy"},'input_2':{classes: "zzz" , id: '"zzz", data-attr: "zzz"}}) }}

Note:Given what we know now I'm sure we can come up with a good format

Nunjucks spread params filter

With that said:

My goal was to end up using a spread params filter which would put the end user in control of all the fields without us needing to define. Perhaps we can utilize this at least in part.

https://github.com/cds-snc/node-starter-app/pull/69#issue-321242317

Happy to discuss some options / ideas ... overall looking for ways to not need to modify the macros when an extra field is needed.

jneen commented 4 years ago

Sounds good, i have a few ideas.

timarney commented 4 years ago

Also FYI -

Let's plan for a major 7.0 release. We can setup a branch.

We would roll out it out sometime after the meeting that Ross scheduled (pending outcomes). Map out a roadmap + features for a major breaking release.

Moving forward -> setup a request for commit proposal system.

cc: @dsamojlenko

jneen commented 4 years ago

@timarney would it be alright to do the radio button attributes bit in a separate PR? this one is getting pretty big as it is (and I have an idea of how to do it in a non-breaking way).

lgtm-com[bot] commented 4 years ago

This pull request introduces 11 alerts when merging 044597e5f6ad6a1097fe2927397d34afe48077b2 into 127d1c2d5cc119d93e437a6987b9feab0e0283d6 - view on LGTM.com

new alerts:

10 for Unused variable, import, function or class
1 for Useless conditional

lgtm-com[bot] commented 4 years ago

This pull request introduces 11 alerts when merging fed458acecdafe6b02fe42608929f228bbbf9fcd into 127d1c2d5cc119d93e437a6987b9feab0e0283d6 - view on LGTM.com

new alerts:

10 for Unused variable, import, function or class
1 for Useless conditional

lgtm-com[bot] commented 4 years ago

This pull request introduces 11 alerts when merging 59009d7ba08b99e8cb5a00680528064f1edeb704 into 127d1c2d5cc119d93e437a6987b9feab0e0283d6 - view on LGTM.com

new alerts:

10 for Unused variable, import, function or class
1 for Useless conditional

lgtm-com[bot] commented 4 years ago

This pull request introduces 1 alert when merging b3396e9da07d6ae23ca0d4d422970c02dc9bdd70 into 127d1c2d5cc119d93e437a6987b9feab0e0283d6 - view on LGTM.com

new alerts:

1 for Useless conditional

timarney commented 4 years ago

alright to do the radio button attributes bit in a separate PR

For sure was just flagging that use case

lgtm-com[bot] commented 4 years ago

This pull request introduces 1 alert when merging 4f1a474fa59cead6f1b51fe3beabec9c29ec9c7c into 127d1c2d5cc119d93e437a6987b9feab0e0283d6 - view on LGTM.com

new alerts:

1 for Useless conditional

jneen commented 4 years ago

@timarney should be green, ready for review. if we want to merge this to a special 7.0 branch that'd be fine too :]

timarney commented 4 years ago

@JuliannaR can you take a look https://cds-node-starter-pr-132.herokuapp.com/en/addresses

dsamojlenko commented 4 years ago

Late to the party here... but wondering if we can implement this in a way where the Nunjucks templates are decoupled from the framework. ie, all necessary attributes are passed into the macro, and don’t require access to functions injected by middleware, or knowledge of where data is coming from (ie, the current iteration has references out to data.[fieldname], or how the macros all know about the errors object)

Perhaps there could be a set of dumb core macros for the individual field types that are wrapped/extended by macros that enable this advanced integration with the framework.

This way the underlying macros would be more portable/reusable in other contexts.

Don't mean to throw a monkey wrench in here... just think this would help with the progression stuff we were talking about earlier. Might also help limiting the breakingness of this major breaking change.

timarney commented 4 years ago

@jneen

Noting some things now as it's the first time we've had access to a preview link (as of yesterday).

Accessibility

@JuliannaR

First thing I found on the a11y front. If you tab to add new address and then tab again to reach the first field in the newly added "sub-form" it hides (the newly created fields) and jumps to the continue button. The fields are added but I could access them and ended up with a bunch of address fields when I reverted to my trackpad + clicking.

The addresses remove fields links don't have unique identifiers .... and I think we'll uncover a bunch more as we go.

Usability Concerns

The above (which I know we haven't spent time on) has me questioning the usability in general of the repeater pattern (not the code implementation). Understanding the need we see based on the 2026 app and I'm sure most of the Gov pdf forms but does this translate well for accessible forms? Have we looked at others using this pattern?

Research & Design

Was user research done on the 2026 app that would indicate this is a good pattern to move forward with?

Also flagging this link for @MithulaNaik https://cds-node-starter-pr-132.herokuapp.com/en/addresses

Appreciate all the work and effort to date, just wanting to make sure we cover all our bases that we want to move forward with this 100%.

cc: @rossferg @cgovias

Also flagging @dsamojlenko comments above so they don't get buried.

macros decoupled from the framework

Would love to see if there's any way to pull this off it would be really nice to be able to use the macros elsewhere if possible i.e. https://www.11ty.io supports Nunjucks and might be a good fit for us to use for static sites (use our macros and styles).

jneen commented 4 years ago

@dsamojlenko Hi! Thanks for taking the time to look at this. My first attempt at building something like this used only nunjucks macros. Unfortunately, nunjucks is extremely restrictive in its scoping rules, to the point that call macros do not even retain access to their containing scope. There is a chance that in the future they will allow dynamic or global variables, and we will be able to refactor this into pure nunjucks. However, for the time being, the only way to consistently change context is to use helper functions injected in a middleware.

jneen commented 4 years ago

Based on whatever research comes out of this, I would suggest we put some design resources into it! As far as it goes now we are only generating fieldset tags with no border, and everything seems to run together. I will take a look at the tabbing issue, hopefully get that fixed, and I should be able to add a unique id to the remove links.

The use case here, to be clear, is when the number of user inputs is potentially unbounded - for example, "all addresses in the last two years", where someone could have had a very large amount of addresses. There may be other ways to solve this problem, but the general "add another" functionality is definitely needed (compare to existing forms, where applicants are asked to design their own attachment format "if there isn't room").

jneen commented 4 years ago

cc @cgovias re: the user research we did in 2620 - I was not able to come as a distributed employee :]

jneen commented 4 years ago

Also @dsamojlenko requiring the form designer to pass in all necessary attributes was something we considered, but I ultimately decided that it unnecessarily increased the complexity burden in something that was going to be repeated many times, and introduced a large possibility of very subtle coding errors. Consider that every field would need to receive not only the current value, but also the error (if present), and a scoped name - and all three of these have subtly different access patterns! For the scoped name, we need addresses[0][attr], for the error addresses[0].attr, and for the value we have to iterate into an actual data structure. This is much easier done behind the scenes.

jneen commented 4 years ago

Nevermind, I found the research document @timarney @JuliannaR https://docs.google.com/document/d/11ZsxDD2twtmwvrIycEJtd2XSF6a0eGnxAwA6HO_XyzM/edit#heading=h.bsucfu4hx5bt

It touches on a few bugs and... the fact that there's no border between fieldsets.

JuliannaR commented 4 years ago

@timarney yes, the remove and add don't unique identifiers so hard to tell which you are removing or adding.

If you hit enter after marking any checkbox it advances you to the next page for with the confirmation of addresses. Spacing is difficult to read/understand that you are interacting with different segments to enter content.

Also, unlikely to translate into accessible forms. Some testing will have to be looked at when it gets there.

JuliannaR commented 4 years ago

@timarney yes, the remove and add don't unique identifiers so hard to tell which you are removing or adding.

If you hit enter after marking any checkbox it advances you to the next page for with the confirmation of addresses. Spacing is difficult to read/understand that you are interacting with different segments to enter content.

Also, unlikely to translate into accessible forms. Some testing will have to be looked at when it gets there.

jneen commented 4 years ago

@JuliannaR I can add an id attribute to all of those - are there other attributes I should add to things to link them?

jneen commented 4 years ago

Also, submitting the form on <enter> seems to be the default behaviour, and is the same on any textbox. Should we override this with javascript? And if so, what behaviour would be expected?

jneen commented 4 years ago

I have added unique id attributes to the repeat and remove-repeat links - the limitation now is there can't be more than one of them (but i don't think that's actually a use-case).

@JuliannaR I did some navigating around with the keyboard and you're absolutely right, the links are pretty hard to use. I've added some javascript code that allows pressing the spacebar to activate them, but I'm not sure this is the correct approach. Would a <button> be more appropriate here?

jneen commented 4 years ago

I have significantly improved keyboard navigation and redone the js includes to work with the new system. Any client.js file in a route directory will be automatically compiled by webpack and included in the page via base.njk, and global/client.js will be included in every page. Shared code is kept in assets/js.

jneen commented 4 years ago

rebased against master.

lgtm-com[bot] commented 4 years ago

This pull request introduces 1 alert when merging daae75f2c15e0eef5046f65d3b3feae300ec22e5 into 7643dce0e07e8000eec0688033116126ba0a898e - view on LGTM.com

new alerts:

1 for Call to eval-like DOM function

jneen commented 4 years ago

The LGTM alert is for the document.write in the polyfill script, which was already present, I just moved it :]

jneen commented 4 years ago

This will 100% need design work, but I don't know how to allocate that. @rossferg, thoughts?

lgtm-com[bot] commented 4 years ago

This pull request introduces 1 alert when merging 10c35e50a947866a1843f806640babbf8483fafd into 7643dce0e07e8000eec0688033116126ba0a898e - view on LGTM.com

new alerts:

1 for Call to eval-like DOM function

rossferg commented 4 years ago

This will 100% need design work, but I don't know how to allocate that. @rossferg, thoughts?

Noted. Will find design support.

jneen commented 4 years ago

Just so everyone knows I'll be out on family-related leave for a good while starting tomorrow, so today is the last day I can shepherd this.

jneen commented 4 years ago

cc @timarney just bumping this because it's the last day I can do any fixes!

lgtm-com[bot] commented 4 years ago

This pull request introduces 1 alert when merging 98928919ad80574533d9391cdd00cf4c43246ab9 into 7643dce0e07e8000eec0688033116126ba0a898e - view on LGTM.com

new alerts:

1 for Call to eval-like DOM function

jneen commented 4 years ago

I should say, another benefit of using getData, getError, and getName is that these are all fully abstract, so they should actually be easier to use in different contexts. If we are using this in a static page, for example, there is no session integration, so we can just return null from getError and getData.

lgtm-com[bot] commented 4 years ago

This pull request introduces 1 alert when merging 8f1de1016e671d29734cf47bd2b53bace7e789b5 into 7643dce0e07e8000eec0688033116126ba0a898e - view on LGTM.com

new alerts:

1 for Call to eval-like DOM function

lgtm-com[bot] commented 4 years ago

This pull request introduces 1 alert when merging 8eb5f9ecbb91ec544add1aecb6ef975db446f864 into 7643dce0e07e8000eec0688033116126ba0a898e - view on LGTM.com

new alerts:

1 for Call to eval-like DOM function

lgtm-com[bot] commented 4 years ago

This pull request introduces 1 alert when merging b5a609aa3019f241a1805cb1e9f47f33afde4230 into 7643dce0e07e8000eec0688033116126ba0a898e - view on LGTM.com

new alerts:

1 for Call to eval-like DOM function

lgtm-com[bot] commented 4 years ago

This pull request introduces 1 alert when merging 62b898a1cdc87e9704dfad02f061ef05eb3c0320 into 7643dce0e07e8000eec0688033116126ba0a898e - view on LGTM.com

new alerts:

1 for Call to eval-like DOM function

lgtm-com[bot] commented 4 years ago

This pull request introduces 1 alert when merging ef0f025c391d43f98f3142e2f367b6a95af55cf2 into 7643dce0e07e8000eec0688033116126ba0a898e - view on LGTM.com

new alerts:

1 for Call to eval-like DOM function

lgtm-com[bot] commented 4 years ago

This pull request introduces 1 alert when merging 76e9d425a31be195e561ae847223ee4338df8339 into 7643dce0e07e8000eec0688033116126ba0a898e - view on LGTM.com

new alerts:

1 for Call to eval-like DOM function

jneen commented 4 years ago

The LGTM bot warning is buggy - if you click through you will see there are 0 alerts.

jneen commented 4 years ago

Thank you! I agree, the RFC process would have helped this significantly.

As for the "coupling" of the macros, do you mean coupling them to the specific implementation of the starter-repo? I would actually argue that this approach is more decoupled, as any system can define getName, getError, and getData and use these immediately (static site generators, for example), rather than having all that logic within handlebars code.

cds-snc / node-starter-app

The repeater! #132

Accessibility

Usability Concerns

Research & Design