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 speeches format #215

Merged
merged 6 commits into from Jan 3, 2017
Merged

Render the speeches format #215

merged 6 commits into from Jan 3, 2017

Conversation

fofr
Copy link
Contributor

@fofr fofr commented Dec 21, 2016

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

Main story:
https://trello.com/c/TWsLTRzc/547-10-speech-migration-mvp-content-schema-examples-and-front-end-work

  • Ports the speeches format from Whitehalls
  • Removes all placeholder images, favouring an empty left column, as used by most formats
  • Handles speakers provided in links hash, and speakers provided as string in details (when they don't have a GOV.UK profile)

Speech sub types

Speech sub-type Schema document type Displays as
Transcript speech Speech
Draft text speech Speech
Speaking notes speech Speech
Written statement to Parliament written_statement Written statement to Parliament
Oral statement to Parliament oral_statement Oral statement to Parliament
Authored article authored_article Authored article

Speaker images

The images are pulled from the profile of the assigned speaker. For now the frontend expects this to come from the image property in the details. It may in future make more sense for it to come via an image property in the links hash of the speaker – as that's the dependency.

Omissions

Screenshots

Feature Old New
Speaker without a profile speaker-without-profile-old speaker-without-profile-new
Authored article authored-article-old authored-article-new
Draft text draft-text-old draft-text-new
Tablet view draft-text-tablet-old draft-text-tablet-new

@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-215 December 21, 2016 15:53 Inactive
@tvararu
Copy link
Contributor

tvararu commented Dec 21, 2016

Solid work.

The speaker image doesn't look like it's full width on tablet in the new version, is that intentional?

updated: @content_item.updated,
history: @content_item.history,
part_of: @content_item.part_of
%>
Copy link
Contributor

Choose a reason for hiding this comment

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

There's 3 different variations of ending tag %> positioning in this file but I don't mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

We do seem to have several different styles of closing %> through the project. I'm not overly bothered, but it might be worth a tidy up at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be fixed by #216

@fofr
Copy link
Contributor Author

fofr commented Dec 21, 2016

@tvararu The image width is intentional yes, when it goes full size it swamps the view a little. This is the current image behaviour on other formats.

Copy link
Contributor

@bevanloon bevanloon left a comment

Choose a reason for hiding this comment

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

Looks good

@@ -0,0 +1,41 @@
<%= content_for :page_class, @content_item.format.dasherize %>
<%= content_for :title, @content_item.title %>

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also include:
<%= content_for :meta_description, @content_item.description %>
?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 99f3f8f

updated: @content_item.updated,
history: @content_item.history,
part_of: @content_item.part_of
%>
Copy link
Contributor

Choose a reason for hiding this comment

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

We do seem to have several different styles of closing %> through the project. I'm not overly bothered, but it might be worth a tidy up at some point.

Run `bundle exec rails generate format speech`
By parsing `page.text` and fetching content, double spaces will always be
stripped and replaced with a single space.

Double spaces have no effect on the rendered output.

Many examples have included double spaces in their body which have led
to tests failing unnecessarily. Make the test a little more lenient in
what it accepts.
The image doesn’t add content to the page. Many formats show an empty
sidebar column instead. This is preferable.

This is the behaviour we want for speeches by people without a GOVUK
profile. Apply it to case studies too.
Speakers without profiles aren’t included in links, because they aren’t
represented as content on GOV.UK. The most notable example is The Queen.

* Insert the speaker into the `from` in the metadata
* Also insert into the footer for consistency
Also use `page_title` method which gets updated when content is
withdrawn
@gpeng gpeng merged commit eb6bf78 into master Jan 3, 2017
@gpeng gpeng deleted the speeches branch January 3, 2017 13:10
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.

None yet

5 participants