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

Follow up questions #304

Merged
merged 7 commits into from
Feb 22, 2017
Merged

Follow up questions #304

merged 7 commits into from
Feb 22, 2017

Conversation

allait
Copy link
Contributor

@allait allait commented Feb 16, 2017

Remove 'is defined' check from textbox form template

hidden question property should only ever be set to True if present
(since the only thing it controls is whether the question should be
initially hidden), so we can remove the separate if hidden is defined
check, which would cause issues with Nunjucks.

Add followup support for radio and checkbox questions

Merges the boolean and radio question form templates into one and
adds support for the multiple followup questions using the new
Question.values_followup property passed in as followup.

@allait allait requested review from tombye and pcraig3 February 16, 2017 17:23
@allait allait force-pushed the follow-up-questions branch from 7bd0aac to 3ce51eb Compare February 16, 2017 17:23
Copy link
Contributor

@tombye tombye left a comment

Choose a reason for hiding this comment

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

This looks ace and I can't see any problems with the implementation but can we have an example of it added to the docs page for selection-buttons?

Copy link
Contributor

@samuelhwilliams samuelhwilliams left a comment

Choose a reason for hiding this comment

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

Can we add the hidden check ({% if hidden %} js-hidden related-information{% endif %}) to list_entry.html please? We need it there, as well.

@allait allait force-pushed the follow-up-questions branch from 3ce51eb to c464e03 Compare February 22, 2017 16:45
@allait
Copy link
Contributor Author

allait commented Feb 22, 2017

@samuelhwilliams added hidden flag for list-entry questions
@tombye added some examples to combinations.html
@pcraig3 I've rebased this on top of current master and had to make some more changes to the selection-buttons.html, would appreciate an additional review to check that I haven't missed anything.

followup: requirement-evidence-110
followup:
true:
- requirement-evidence-110
Copy link
Contributor

@samuelhwilliams samuelhwilliams Feb 22, 2017

Choose a reason for hiding this comment

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

This should be the other way around, shouldn't it?

followup:
  requirement-evidence-110:
    - true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not YAML format used by the content loader, it's the data we'd normally pass into toolkit form macros, so this is what .values_followup would return, which is a reversed mapping.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok

`hidden` question property should only ever be set to `True` if present
(since the only thing it controls is whether the question should be
initially hidden), so we can remove the separate `if hidden is defined`
check, which would cause issues with Nunjucks.
Merges the boolean and radio question form templates into one and
adds support for the multiple followup questions using the new
`Question.values_followup` property passed in as `followup`.
…#383

We have to ship the file separately until the GOV.UK frontend toolkit
PR is merged.
@allait allait force-pushed the follow-up-questions branch from c464e03 to ed4984f Compare February 22, 2017 17:19
Also updates `followup` format in page examples
@allait allait force-pushed the follow-up-questions branch from ed4984f to 5172528 Compare February 22, 2017 17:28
@allait allait force-pushed the follow-up-questions branch from 5172528 to 74eada6 Compare February 22, 2017 17:40
@@ -13,7 +13,7 @@
{% elif message %}
<div class="message-wrapper">
{% endif %}
<fieldset class="question {% if first_question %}first-question{% endif %}" id="{{ id }}">
<fieldset class="question {% if first_question %}first-question{% endif %}{% if hidden %} js-hidden related-information{% endif %}" id="{{ id }}">
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a thing we've done in a few places where we add extra spaces to classnames.
If you remove the space after class="question and put it before first-question, it's more correct.

{% endif %}

{% for option in options %}
{% set input_value = option.value if 'value' in option else option.label %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Good call fixing this stuff. I know @benvand was asking about it.

@pcraig3
Copy link
Contributor

pcraig3 commented Feb 22, 2017

I think go for it. 👍

There's some crappy grey borders in there, but we can patch that in the next couple days, I think.

@allait allait merged commit 078c5a4 into master Feb 22, 2017
@allait allait deleted the follow-up-questions branch February 22, 2017 18:21
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.

4 participants