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 "Take part" format #69

Merged
merged 8 commits into from Jan 20, 2016
Merged

Render "Take part" format #69

merged 8 commits into from Jan 20, 2016

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Jan 19, 2016

https://trello.com/c/P1IYhe0C/242-take-part-migrate-front-end-work-large

  • Add Take Part template, styles and presenter
    • Use format rather than format_display_type for gathering title context
    • Copy markup patterns from case study where similar/the same
  • Don’t port the strange “underline” take part headings, the take part contents are purposefully left out, and the description has been moved above the image to match other formats.
  • Extract styles and markup common to Case Studies and Take Part into partials and mixins
  • When an image is omitted it will show a generic government placeholder image

Before

screen shot 2016-01-19 at 16 22 09

After

screen shot 2016-01-19 at 15 31 13

cc @dsingleton @boffbowsh @benlovell @gpeng

@boffbowsh
Copy link
Contributor

@boffbowsh boffbowsh commented Jan 19, 2016

👍 for ones and zeroes, I'll let someone else review the shapes and colours

@dsingleton
Copy link
Contributor

@dsingleton dsingleton commented Jan 19, 2016

LGTM.

Note, @fofr and I discussed this offline: As we add more formats we're going to need a pattern to avoid duplication of presenters, templates, and SASS. I'm not sure the approach of composable partials (and shared SASS mixins) will be right long term, but it will work for this format and the next few simple ones.

As we add more formats we'll have a better idea of the duplication pattern and can re-evaluate the approach. But in the mean time i'm happy to press on with this approach to get us moving 👍

@benlovell
benlovell reviewed Jan 19, 2016
View changes
test/presenters/take_part_presenter_test.rb Outdated
require 'test_helper'

class TakePartPresenterTest < ActiveSupport::TestCase
include ActionView::Helpers::UrlHelper

This comment has been minimized.

@benlovell

benlovell Jan 19, 2016
Contributor

Is this needed?

This comment has been minimized.

@fofr

fofr Jan 20, 2016
Author Contributor

You're right, removed and rebased.

@benlovell
benlovell reviewed Jan 19, 2016
View changes
test/presenters/take_part_presenter_test.rb Outdated
end

private

This comment has been minimized.

@benlovell

benlovell Jan 19, 2016
Contributor

We should drop this line per the style guide.

This comment has been minimized.

@fofr

fofr Jan 20, 2016
Author Contributor

Updated this test file and the existing case study ones to match style guide.

@benlovell
benlovell reviewed Jan 19, 2016
View changes
test/presenters/take_part_presenter_test.rb Outdated

private

def presented_take_part(overrides={})

This comment has been minimized.

@benlovell

benlovell Jan 19, 2016
Contributor

Can you add spaces around the =, per the style guide?

I am quickly becoming the human embodiment of our style guide. 😊

This comment has been minimized.

@fofr

fofr Jan 20, 2016
Author Contributor

Done 👍

@benlovell
benlovell reviewed Jan 19, 2016
View changes
app/views/shared/_sidebar_image.html.erb Outdated
@@ -0,0 +1,11 @@
<% image ||= false %>

This comment has been minimized.

@benlovell

benlovell Jan 19, 2016
Contributor

nil is falsey, so I think this may be unnecessary.

This comment has been minimized.

@fofr

fofr Jan 20, 2016
Author Contributor

I've removed this line. I think the problem this solves is if image isn't passed in as a local variable. Given this is a sidebar_image partial, that shouldn't ever happen.

fofr added 6 commits Jan 18, 2016
* Use `format` rather than `format_display_type` for gathering title
context
* Copy markup patterns from case study where similar/the same
* Don’t port the strange “underline” take part headings, the take part
contents are purposefully left out, and the description has been moved
above the image to match other formats.
* There is some duplication between case studies and take part at the
moment
Start with common parts, then apply view specific overrides later.
Putting helpers first helps keep specificity low.
* Extract common styles into a mixin
* Extract common markup into partials

In the format template it’s still easy to view the sections of the page
as the partials never occupy more than a single “row”.
Keep the styles separate, as per the partials, so they remain distinct
and compose-able.
This list is buried deep within a test and could have been easily
missed. A canonical list of supported features would make more sense.
* Consistently apply a responsive bottom margin
* Apply this to govuk-metadata, description and sidebar-with-image
* Remove the now redundant top margin on descriptions in case studies
@fofr fofr force-pushed the take-part branch to 4bdf623 Jan 20, 2016
benlovell added a commit that referenced this pull request Jan 20, 2016
Render "Take part" format
@benlovell benlovell merged commit c08044a into master Jan 20, 2016
1 check passed
1 check passed
default Build #233 succeeded on Jenkins
Details
@benlovell benlovell deleted the take-part branch Jan 20, 2016
@benlovell
Copy link
Contributor

@benlovell benlovell commented Jan 20, 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.