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 "Topical event about page" format #92

Merged
merged 10 commits into from Feb 10, 2016
Merged

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Feb 5, 2016

  • Purposefully alter the topical event about page to look more like other formats. Move the description up beneath the title, introduce a breadcrumb, add spacing beneath body, remove underlined heading styles in main content
  • Port dash-list styles from Whitehall for contents rendering
  • Add helper for generating a list of contents based on h2 headings within format's body
  • To keep the design consistent with what’s currently live, introduce an offset class which is used only when there is no sidebar.
  • Indicate when a topical event is archived in the breadcrumb

https://trello.com/c/P4KCL1UJ/267-3-topical-event-about-page-front-end-work-medium

Before

ebola-old

After

ebola


Before

somme-old

After

somme

fofr added 2 commits Jan 28, 2016
Add files for styles, template, presenter and tests
Used for sidebar contents
@fofr fofr force-pushed the topical-event-about-page branch Feb 5, 2016
@benilovj
benilovj reviewed Feb 9, 2016
View changes
app/controllers/content_items_controller.rb Outdated
when 'statistics_announcement' then StatisticsAnnouncementPresenter.new(content_item)
when 'take_part' then TakePartPresenter.new(content_item)
when 'topical_event_about_page' then TopicalEventAboutPagePresenter.new(content_item)

This comment has been minimized.

@benilovj

benilovj Feb 9, 2016
Contributor

It feels to me like there's enough duplication here to use convention over configuration (ie to derive the presenter class name from the format name):

def present(content_item)
  presenter_name = content_item['format'].classify + 'Presenter'
  presenter_class = Object.const_get(presenter_name)
  presenter_class.new(content_item)
rescue NameError
  raise "No support for format \"#{content_item['format']}\""
end

This comment has been minimized.

@benlovell

benlovell Feb 9, 2016
Contributor

Much nicer

String#constantize wraps the Module.const_get dance (but doesn't respect lexical context) if you go with that approach.

This comment has been minimized.

@fofr

fofr Feb 10, 2016
Author Contributor

Nice, I've updated.

@benilovj
benilovj reviewed Feb 9, 2016
View changes
app/presenters/extracts_headings.rb Outdated
@@ -0,0 +1,10 @@
module ExtractsHeadings
def extract_headings_with_ids(html)
[].tap do |headings|

This comment has been minimized.

@benilovj

benilovj Feb 9, 2016
Contributor

I think you can get rid of one level of nesting with the following:

module ExtractsHeadings
  def extract_headings_with_ids(html)
    Nokogiri::HTML(html).css('h2').map do |heading|
      id = heading.attribute('id')
      { text: heading.text, id: id.value } if id
    end.compact
  end
end

This comment has been minimized.

@benlovell

benlovell Feb 9, 2016
Contributor

❤️ aside: why is there no Enumerable#compact_map?

This comment has been minimized.

@fofr

fofr Feb 10, 2016
Author Contributor

@benilovj This doesn't meet our style criteria:
app/presenters/extracts_headings.rb:6:5: C: Style/MethodCalledOnDoEndBlock: Avoid chaining a method call on a do...end block. (https://github.com/bbatsov/ruby-style-guide#single-line-blocks) end.compact

I think this is an ok exception to the rules listed here: https://github.com/bbatsov/ruby-style-guide#single-line-blocks

This comment has been minimized.

@benlovell

benlovell Feb 10, 2016
Contributor

Using braces, since this block returns a value, should be OK.

This comment has been minimized.

@fofr

fofr Feb 10, 2016
Author Contributor

Updated to use braces.

@benilovj
benilovj reviewed Feb 9, 2016
View changes
app/views/shared/_sidebar_contents.html.erb Outdated
@@ -0,0 +1,12 @@
<% if contents.any? %>
<nav role="navigation">

This comment has been minimized.

@benilovj

benilovj Feb 9, 2016
Contributor

should this be indented (due to the if statement above)?

This comment has been minimized.

@fofr

fofr Feb 10, 2016
Author Contributor

Added indentation

@benilovj
benilovj reviewed Feb 9, 2016
View changes
test/test_helper.rb Outdated
end
end

def assert_has_contents(contents)

This comment has been minimized.

@benilovj

benilovj Feb 9, 2016
Contributor

this method name sounds very generic but does something very specific (what I mean is contents could also be taken to mean 'page contents', and this method is a generic test helper).

This comment has been minimized.

@fofr

fofr Feb 9, 2016
Author Contributor

Good point, I'll update to contents-list

This comment has been minimized.

@fofr

fofr Feb 10, 2016
Author Contributor

Done in 38ef266

@benilovj
benilovj reviewed Feb 9, 2016
View changes
test/presenters/extracts_headings_test.rb Outdated

test "ignores headings without an id" do
html = '<h2>John Doe</h2>'
assert_equal [], extract_headings_with_ids(html)

This comment has been minimized.

@benilovj

benilovj Feb 9, 2016
Contributor

Personally, I think assert_empty extract_headings_with_ids(html) would read better...

This comment has been minimized.

@fofr

fofr Feb 10, 2016
Author Contributor

I agree, updated

@benilovj
benilovj reviewed Feb 9, 2016
View changes
test/integration/topical_event_about_page_test.rb Outdated

test "breadcrumbs show whether a topical event is archived" do
@content_item = JSON.parse(get_content_example("topical_event_about_page")).tap do |item|
content_store_has_item(item["base_path"], item.to_json)

This comment has been minimized.

@benilovj

benilovj Feb 9, 2016
Contributor

Is there much point doing the tap call, as opposed to just a sequential statement?

@content_item = JSON.parse(get_content_example("topical_event_about_page"))
content_store_has_item(@content_item["base_path"], @content_item.to_json)

This comment has been minimized.

@fofr

fofr Feb 10, 2016
Author Contributor

Good point, removed.

@benlovell
benlovell reviewed Feb 9, 2016
View changes
Gemfile Outdated
@@ -23,6 +23,7 @@ end
group :development, :test do
gem 'govuk-lint'
gem 'wraith', '~> 3.1'
gem 'timecop', '0.8.0'

This comment has been minimized.

@benlovell

benlovell Feb 9, 2016
Contributor

Rails has travel_to built in since 4.1 which has feature-parity with timecop and doesn't require yet another dependency.

This comment has been minimized.

@fofr

fofr Feb 10, 2016
Author Contributor

Now removed

@benlovell
benlovell reviewed Feb 9, 2016
View changes
app/presenters/topical_event_about_page_presenter.rb Outdated
title = archived_topical_event? ? "#{parent['title']} (Archived)" : parent["title"]

[
{title: "Home", url: "/"},

This comment has been minimized.

@benlovell

benlovell Feb 9, 2016
Contributor

A general style point: can we add spaces inside of the hash braces throughout?

This comment has been minimized.

@benilovj

benilovj Feb 9, 2016
Contributor

I was wondering – how come linting doesn't pick that up?

This comment has been minimized.

This comment has been minimized.

@fofr

fofr Feb 10, 2016
Author Contributor

Fixed in f56cc2e

@fofr fofr force-pushed the topical-event-about-page branch Feb 10, 2016
fofr added 6 commits Jan 28, 2016
A top level headings list is needed to generate a table of content with
links to their IDs.
Purposefully alter the topical event about page to look more like other
formats. Move the description up, beneath the title, introduce a
breadcrumb, add spacing beneath body.

* Create a sidebar contents partial
* Use heading extraction from content item body to generate contents
* Add integration and presenter tests for contents and breadcrumbs
When no contents are shown the grid wrapper is empty. Because it has no
height it collapses and the content shifts to the left.

To keep the design consistent with what’s currently live, introduce an
offset class which is used only when there is no sidebar.

Grid helpers like this are best used in the front-end toolkit, but
while this is a one-off it makes sense to put it here.
The topical event start and end dates from from the about page’s
parent, in its details hash. Start and end dates are optional.

* Date range is exclusive of end date (meaning Date and DateTime tests
are equivalent), as per Whitehall
* Include time based integration and unit tests for breadcrumb output
Use convention over configuration.
@benilovj
benilovj reviewed Feb 10, 2016
View changes
app/presenters/extracts_headings.rb Outdated
Nokogiri::HTML(html).css('h2').map { |heading|
id = heading.attribute('id')
{ text: heading.text, id: id.value } if id
}.compact

This comment has been minimized.

@benilovj

benilovj Feb 10, 2016
Contributor

To bring it into the house style, you can change this to:

headings = ...
headings.compact
@fofr fofr force-pushed the topical-event-about-page branch Feb 10, 2016
@fofr
Copy link
Contributor Author

@fofr fofr commented Feb 10, 2016

Updated and rebased. I've taken on most of Jake's suggestions, removed Timecop, upgraded gouvk-lint and fixed the style offences.

fofr added 2 commits Feb 10, 2016
* Add spacing within hashes
* Don’t chain do … end blocks
* Remove unnecessary `{ }` wrappers
“Contents” is an ambiguous term
@fofr fofr force-pushed the topical-event-about-page branch to ec93e2d Feb 10, 2016
benilovj added a commit that referenced this pull request Feb 10, 2016
Render "Topical event about page" format
@benilovj benilovj merged commit 4d81f43 into master Feb 10, 2016
1 check passed
1 check passed
default Build #395 succeeded on Jenkins
Details
@benilovj benilovj deleted the topical-event-about-page branch Feb 10, 2016
@benlovell
Copy link
Contributor

@benlovell benlovell commented Feb 10, 2016

💖 🚀

@dsingleton

This comment has been minimized.

Copy link
Contributor

@dsingleton dsingleton commented on app/controllers/content_items_controller.rb in 7b64876 Feb 10, 2016

This could really do with a test to support it, and maybe be split out to a helper method for readability?

This comment has been minimized.

Copy link
Contributor

@benilovj benilovj replied Feb 10, 2016

This would fail quite loudly at the integration level if it were wrong – what risk would the test mitigate?

This comment has been minimized.

Copy link
Contributor

@benlovell benlovell replied Feb 10, 2016

You could simplify this a little with presenter_class.constantize.new(content_item).

This comment has been minimized.

Copy link
Contributor

@dsingleton dsingleton replied Feb 10, 2016

This would fail quite loudly at the integration level if it were wrong – what risk would the test mitigate?

Thats true, although a unit test would do more to self-document whats going, given it's quite a terse few lines. Refactoring/moving it out to a helper would probably be good enough

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.