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

Create a need view #28

Merged
merged 30 commits into from Nov 19, 2013
Merged

Create a need view #28

merged 30 commits into from Nov 19, 2013

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Nov 15, 2013

  • Link to need view from need ID and formatted need goal in listing
  • Create a view that puts formatted need foremost
  • Break down evidence into secondary interaction numbers and legislation and further evidence sections
  • List impact and justifications
  • Loosely base styles on GOV.UK styles (font-sizes, colours, border widths), with the accepted limitation of running on bootstrap and without using New Transport
  • Break down needs CSS into separate files
  • Format interaction numbers based on content style guide, be clear numbers are approximations
  • Introduce a retina image mixin
JordanHatch and others added 29 commits Nov 8, 2013
Allow a need to be stubbed based on a JSON file in
the test/fixtures/needs directory.
No longer redirect the 'show' action to 'edit',
instead displaying some basic information about
the need and a link to the edit form.

This tidies up the needs controller test a bit and
updates the integration tests to update a need so
they can still navigate to the edit form.
* Create need view prototype
* Demark need in a bootstrap well at the top of the view
* Include sections for evidence, justification and impact
* Use government departmental colours as defined on
https://www.gov.uk/service-manual/user-centered-design/resources/colour-
palettes.html
* Style evidence in the GOV.UK 'big number' manner
Wrap the need in an article div, and extract many
of the Bootstrap classes out of the markup into
better semantically named rules.
Conflicts:
	test/integration/update_a_need_test.rb
When organisations are returned from the Need API
for a need, include them in the need and allow them
to be accessed as an array of OpenStructs.
In the view mode, we want to use a slightly rephrased
form of the impact strings so that they fit into
the sentence "If GOV.UK didn't meet this need…".

To achieve this, a new helper method is added to the
need helper which converts the existing impact into
a key which is then looked up in the locale file.
Display all the organisations for a need above the
goal. Each org is separated by a comma and space.
Use the Need's attributes to populate the remaining
bits of data in the need view. 

I've added a second need fixture response to assist
testing when there are empty values returned from
the Need API.

This involves adding some new need helper methods 
to format numbers and percentages in a useful way.
* Add need ID to breadcrumb
* Add a history button to header and remove revisions from foot of page
* Restyle impact statement based on GOV.UK help notice
* Include legislation and further evidence
* Rename evidence to interactions
Move recent changes from the show view to a new
page at /needs/<id>/revisions.
Rename the view helper 'show_evidence_column?' 
too.
Use a shared partial called "need_header" to display
the organisations, need goal and edit/history buttons
consistently between need views.
Use the 'important' icon from GOV.UK to highlight
the need's impact.
* Partition theme into sections with comments
* Create files for form, revisions and need
* Break up pre-refactor
* Remove p.new-need which wasn't being used
* Generally avoid fighting against bootstrap with extra layers of
abstraction
* Move theme and variable imports into application, for inclusion
everywhere
* Improve margin helper classes to avoid specificity problems
* Improve semantics of header partial and alignment of button group
* Use bootstrap breadcrumb with added margin
- Rename icon-important
- Add new standard size icon
- Add new mixins helper file
- Remove background size and retina image, instead use both fallback
standard image and retina image
- Add mixins for rem and em calculations - to be used later
* Use bootstrap variables for margins and line heights
* Reduce indentation and specificity
* Make selector intent clearer
* Make tests less brittle
* Interactions are historical, not live
* Numbers are estimates, but not predictions
@JordanHatch

This comment has been minimized.

Copy link
Contributor

@JordanHatch JordanHatch commented on app/views/needs/_need_header.html.erb in d3441f3 Nov 18, 2013

Is it standard practice to use classes like add-bottom-margin directly in markup?

This comment has been minimized.

Copy link
Contributor

@fofr fofr replied Nov 18, 2013

I've found the class to be incredibly useful. One of the most common reasons for creating a class is to add/remove margins. This saves having to write a new class with new CSS every time you want to tweak margins on elements - keeping CSS manageable. It also keeps margins in a consistent vertical rhythm and the class is unambiguous.

In some cases, ie when we're already applying custom styles, it makes sense to bundle the margins up into that class, preferably with an extend or mixin to keep margins consistent. In this case we want to keep the base class of block-border quite light, in the future it may make sense to bundle these two together.


@mixin image-2x($image, $width, $height) {
@media (min--moz-device-pixel-ratio: 1.3),
(-o-min-device-pixel-ratio: 2.6/2),

This comment has been minimized.

@bishboria

bishboria Nov 19, 2013
Contributor

Is this different from the others? 2.6/2 is 1.3.

This comment has been minimized.

@gemmaleigh

gemmaleigh Nov 19, 2013
Contributor

http://dev.opera.com/articles/view/an-introduction-to-meta-viewport-and-viewport/

Note that Opera only accepts ratios as device-pixel-ratio values (e.g. 3/2). The default Android browser on the other hand prefers numbers (e.g. 1.5). As this feature requires a vendor prefix at this point, it is just a matter of specifying the right value for each rendering engine.

On Tuesday, 19 November 2013 at 07:48, Stuart Gale wrote:

In app/assets/stylesheets/_mixins.scss:

@@ -0,0 +1,25 @@ > +// Retina Images mixin > + > +@mixin image-2x($image, $width, $height) { > + @media (min--moz-device-pixel-ratio: 1.3), > + (-o-min-device-pixel-ratio: 2.6/2),
Is this different from the others? 2.6/2 is 1.3.


Reply to this email directly or view it on GitHub (https://github.com/alphagov/maslow/pull/28/files#r7750356).

def format_need_goal(goal)
return "" if goal.blank?

words = goal.split(" ")
words.first[0] = words.first[0].upcase
words.join(" ")

This comment has been minimized.

@bishboria

bishboria Nov 19, 2013
Contributor

we could replace these three lines with: goal[0] = goal[0].upcase then return goal.

end

def show_interactions_column?(need)
[ need.monthly_user_contacts, need.monthly_site_views, need.monthly_need_views, need.monthly_searches ].select(&:present?).any?

This comment has been minimized.

@bishboria

bishboria Nov 19, 2013
Contributor

This line is quite long, could it be wrapped at the array commas?

def calculate_percentage(numerator, denominator)
return unless numerator.present? and denominator.present?

percent = numerator.to_f / denominator.to_f * 100.0

This comment has been minimized.

@bishboria

bishboria Nov 19, 2013
Contributor

The percentage is Inf when the denominator is 0… there's a missing test case for this too.

You're already returning nil if the denom is nil, extend this condition to include 0?

should "return nil if any provided values are nil" do
assert_nil calculate_percentage(10, nil)
assert_nil calculate_percentage(nil, 10)
end

This comment has been minimized.

@bishboria

bishboria Nov 19, 2013
Contributor

as commented above, missing test case when the denominator is 0.

bishboria added a commit that referenced this pull request Nov 19, 2013
@bishboria bishboria merged commit 38d3177 into master Nov 19, 2013
@bishboria bishboria deleted the view-need branch Nov 19, 2013
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.