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

Render the consultation format #201

Merged
merged 13 commits into from Dec 1, 2016
Merged

Render the consultation format #201

merged 13 commits into from Dec 1, 2016

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Nov 29, 2016

Tests will fail until the corresponding schema changes have been merged and deployed: alphagov/govuk-content-schemas#445

Main story:
https://trello.com/c/QseqtCaJ/509-consultations-migration-basic-content-schema-examples-and-front-end-work-large

Ports the following features from Whitehall:

  • Display of consultation description (the body in content item)
  • Display of consultation summary (the description in content item)
  • Open and closing date and date ranges
    • Includes fix for showing an unopened consultation as "opening on" rather than already closed
    • Uses a legible date format, "10 June 2015 10:00am" is now "10am on 10 June 2015"
    • When the time is 12:00am, it shows midday
    • When the time is midnight it either hides the time for opening date or rolls back 1 minute for closing – this remove ambiguity
  • A list of supporting documents using the same pattern as publications
  • A ways to respond section
    • Only displays when open and when a respond online link, email or postal address have been provided. This matches existing logic. If an attachment is uploaded but no email or postal address are provided to return the completed copy it will not display. This is how it is in Whitehall but could be improved.
  • An outcomes section
  • An outcome documents section
  • A public feedback section
  • A public feedback documents section
  • A notice displaying when a consultation is unopened, pending feedback, or closed with outcome
  • National applicability, history mode, withdrawable
  • Includes fix for publication titles at thin viewports, the same bug existed for consultations
  • Renames display_time to display_date as the format provided does not include time
  • Fixes display and presentation of unopened consultations: https://trello.com/c/GEyT6hzk/518-make-it-obvious-when-a-consultation-starts-small
  • Consultation banner colour has been switched from purple to blue

Ways to respond problems

Port of existing copy which isn’t great. Fixes the problem of "Respond online" looking like a heading, instead using the call to action pattern. Described here: https://trello.com/c/pEccXKHV/517-issue-with-ways-to-respond-section-in-consultations-medium

This highlights a problem with how we render content: this HTML is crafted from values provided by users, rather than govspeak – yet it is still content and must be rendered as content using the same styles. For now this means passing that HTML to the govspeak content for rendering.

An alternative would be some free text the user provides in markdown. This would make the format more flexible but less uniform.

Headings structure

This has been ported from Whitehall, but has accessibility problems. We use h1 for each section heading – this is the only way to guarantee that we provide a heading for dynamic content that appears in markdown – eg users could use ## level 2 headings.

However, for a closed consultation we add an original consultation heading, which should wrap the whole of the original consultation. This is currently an h2. It could be an h1 but that would have the same problem. I think this should be looked at separately to this PR.

Whitehall

If the migration of this format takes a while, which given its complexity I suspect it will, the fixes to this format should be back ported to Whitehall. ie readable date formats, blue box, correct handling of an unopened consultation.

Omissions

Screenshots

Feature Old New
Open consultation open_consultation_old open_consultation_new
Closed consultation closed_consultation_old closed_consultation_new
Unopened consultation unopened_consultation_old unopened_consultation_new
Ways to respond ways_to_respond_consultation_old ways_to_respond_consultation_new
Pending feedback pending_feedback_consultation_old pending_feedback_consultation_new
Mobile consultation_mobile_old consultation_mobile_new
fofr added 5 commits Nov 18, 2016
Run: `bundle exec rails generate format consultation`
display_time never displays a time, just a date.

Consultations need to show a human readable date with time.
* Show open, closed and consultations with outcomes
* Style and render consultation notices for closed consultations
pending an outcome and those that have concluded
* Create styles for a consultation banner, showing dates of opening and
closing and the consultation summary (description)
* Include blocks for documents, description (body), outcome detail,
outcome documents

It does not include public feedback, ways to respond, share links, or
correct date formatting.
* Make date ranges readable
* 4:00pm changed to 4pm
* 12:00am changed to 23:59pm the night before
* 12:00pm changed to midday
* Put time, then date
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-201 Nov 29, 2016 Inactive
@nickcolley
Copy link
Contributor

@nickcolley nickcolley commented Nov 29, 2016

For the issues you've mentioned can you raise them as a separate issue so we can track it and resolve it later?

@fofr
Copy link
Contributor Author

@fofr fofr commented Nov 29, 2016

@nickcolley I will do when this gets merged.

}
}

.consultation-notice {

This comment has been minimized.

@andrewgarner

andrewgarner Nov 29, 2016
Contributor

This shouldn't need to be nested within .consultation given the .consultation- prefix?

This comment has been minimized.

@fofr

fofr Nov 29, 2016
Author Contributor

The current pattern is to have all styles for a format nested within their format name.

This comment has been minimized.

@andrewgarner

andrewgarner Nov 29, 2016
Contributor

Fair enough. As someone new to the code base it wasn't clear if we've adopted a BEM type methodology.

<h2>
We are analysing your feedback
</h2>
<p>Visit this page again soon to download the outcome to this public&nbsp;feedback.</p>

This comment has been minimized.

@andrewgarner

andrewgarner Nov 29, 2016
Contributor

There is subtle presentational behaviour here that is mixed in with the content. How about the following?

<style>
  .nowrap { white-space: nowrap; }
</style>

<p>Visit this page again soon to download the outcome to this <span class="nowrap">public feedback.</span</p>

This comment has been minimized.

This comment has been minimized.

@andrewgarner

andrewgarner Nov 29, 2016
Contributor

👍

If that's the approach adopted elsewhere then yes we should use that here too. We're still maybe mixing content and presentation so maybe that's something to consider another time.

<div class="grid-row sidebar-with-body">
<div class="column-third">
<h1 class="section-title" id="final-outcome-detail-title">
<%= t("consultation.final_outcome_detail") %>

This comment has been minimized.

@andrewgarner

andrewgarner Nov 29, 2016
Contributor

We're not consistently translating strings in this file. Should for example "Download the full outcome", "This consultation has concluded" also be translated?

This comment has been minimized.

@fofr

fofr Nov 29, 2016
Author Contributor

This format isn't translated. I think the simplest thing to do would be to not translate any. Some of the date formatting isn't easily translatable in its current form. What do you think?

This comment has been minimized.

@andrewgarner

andrewgarner Nov 29, 2016
Contributor

👍

That would be clearer. At the moment it's not clear if translations are supported or not. Also relevant is how we're using page_text_direction in some places.

This comment has been minimized.

@fofr

fofr Nov 29, 2016
Author Contributor

Fixed in 185a52a

<div class="grid-row">
<div class="column-third consultation-dates">
<% if @content_item.closed? %>
<p>

This comment has been minimized.

@andrewgarner

andrewgarner Nov 29, 2016
Contributor

You don't need to duplicate the <p></p> tags as they remain consistent for each case.

This comment has been minimized.

@fofr

fofr Nov 29, 2016
Author Contributor

Fixed in c19c2cd

assert page.has_text?("closes at 4pm on 16 December 2216")
end

test "un-opened consultation" do

This comment has been minimized.

@andrewgarner

andrewgarner Nov 29, 2016
Contributor

"future consultation"?

This comment has been minimized.

@fofr

fofr Nov 29, 2016
Author Contributor

I prefer unopened. Had a similar discussion with @binaryberry. Unopened goes with open and close. Future would go with past and present. Like an unopened letter.

This comment has been minimized.

@andrewgarner

andrewgarner Nov 29, 2016
Contributor

Fair enough. In that case I'm not sure we need the hyphen. "an unopened consultation".

See http://dictionary.cambridge.org/dictionary/english/unopened

This comment has been minimized.

@fofr

fofr Nov 29, 2016
Author Contributor

Fixed and rebased. I was about to suggest the same 😄

@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-201 Nov 29, 2016 Inactive
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-201 Nov 29, 2016 Inactive
fofr added 8 commits Nov 25, 2016
Make it clear when a consultation hasn’t yet opened, and when it will
open.
When public feedback is added the detail will always be present.
Attachments are optional.
Section titles didn’t have a bottom margin when content collapsed. This
lack of whitespace impacted the legibility of content.
In context of when something opens the options are:

Opens at 12am on 10 June 2016
Opens at 11:59pm on 10 June 2016
Opens on 10 June 2015

The third option is most easily understood and correct.
Only displays when open and when a respond online link, email or postal
address have been provided. This matches existing logic. If an
attachment is uploaded but no email or postal address are provided to
return the completed copy it will not display. This is how it is in
Whitehall but could be improved.

Port of existing copy which isn’t great. Fixes the problem of "Respond
online" looking like a heading, instead using the call to action
pattern. Described here:
https://trello.com/c/pEccXKHV/517-issue-with-ways-to-respond-section-in-
consultations-medium

This highlights a problem with how we render content: this HTML is
crafted from values provided by users, rather than govspeak – yet it is
still content and must be rendered as content using the same styles.
For now this means passing that HTML to the govspeak content for
rendering.

An alternative would be some free text the user provides in markdown.
This would make the format more flexible but less uniform.
Consultations aren’t translated.
This is the heading for the section. The section contains `h1`s which
is a known problem discussed in the PR. This change attempts to reduce
the effect of that problem.
@andrewgarner
Copy link
Contributor

@andrewgarner andrewgarner commented Nov 29, 2016

Upstream the "state" of a consultation is all held within a single schema. We've chosen that path due to the way we integrate with our search index but we shouldn't let that leak down into our frontend.

We could lift out the state of a consultation so that the rules and things that change between each state are more clear. For example we could have an OpenConsultationPresenter and a ClosedConsultationPresenter and a factory method to instantiate the correct one. We could also nest our view code giving us less conditionals.

These are enhancements however so we shouldn't block this PR to think about them.

@andrewgarner
Copy link
Contributor

@andrewgarner andrewgarner commented Nov 29, 2016

@fofr in the example screenshots the "Unopened consultation" example has documents listed in the before screenshot but not in the after screenshot. Is that a deliberate change?

@fofr
Copy link
Contributor Author

@fofr fofr commented Nov 29, 2016

@andrewgarner That's an accidental omission from the example.

@fofr fofr changed the title [DO NOT MERGE] Render the consultation format Render the consultation format Dec 1, 2016
@fofr fofr merged commit 81d372b into master Dec 1, 2016
1 check passed
1 check passed
default Build #969 succeeded on Jenkins
Details
@fofr fofr deleted the add-consultations branch Dec 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.