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.

Sign up for GitHub

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

Jump to bottom

Fixes #476 - Adds missing location details #516

Merged
merged 2 commits into from
Jul 30, 2014
Merged

Fixes #476 - Adds missing location details #516

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2de827e
Fixes #476 - Adds missing location details
anselmbradford Jul 26, 2014
1a7a41c
Use separate partials for each Service field.
monfresh Jul 30, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions app/assets/stylesheets/_base.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1676,6 +1676,11 @@ body { top: 0px !important; }
padding: 35px;
padding-top: 0px;
padding-bottom: 20px;

// Indentation for service category hierarchy.
.hierarchy-depth-1{ margin-left: 10px; }
.hierarchy-depth-2{ margin-left: 20px; }
.hierarchy-depth-3{ margin-left: 30px; }
}

> section > section > section
Expand Down
2 changes: 2 additions & 0 deletions app/controllers/organizations_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ def show

if @org[:services].present?
@categories = @org.services.map { |s| s[:categories] }.flatten.compact
@keywords = @org.services.map { |s| s[:keywords] }.flatten.compact.uniq
@service_areas = @org.services.map { |s| s[:service_areas] }.flatten.compact.uniq
end

# To disable or remove the Result list button on details page
Expand Down
4 changes: 2 additions & 2 deletions app/views/component/detail/_accessibility.html.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
- if org[:accessibility].present?
%section{:class=>css_class_format('accessibility')}
%section{ class: css_class_format('accessibility') }
- if defined? iconclass
%i{:class=>iconclass}
%i{ class: iconclass }
%h1 Accessibility Options:
%ul
- org.accessibility.sort.each do |accessibility_option|
Expand Down
4 changes: 2 additions & 2 deletions app/views/component/detail/_languages.html.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
- if org[:languages].present?
%section{:class=>css_class_format('languages')}
%section{ class: css_class_format('languages') }
- if defined? iconclass
%i{:class=>iconclass}
%i{ class: iconclass }
%h1 Languages Spoken:
%ul
- org.languages.sort.each do |lang|
Expand Down
8 changes: 4 additions & 4 deletions app/views/component/detail/_mail_address.html.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,16 @@
= org.mail_address.attention
%br

%span.street-address{:itemprop=>"streetAddress"}
%span.street-address{ itemprop: 'streetAddress' }
= superscript_ordinals(org.mail_address.street)
%br

%span.city{:itemprop=>"addressLocality"}><
%span.city{ itemprop: 'addressLocality' }><
= org.mail_address.city
= ','

%span.state{:itemprop=>"addressRegion"}
%span.state{ itemprop: 'addressRegion' }
= org.mail_address.state

%span.zipcode{:itemprop=>"postalCode"}
%span.zipcode{ itemprop: 'postalCode' }
= org.mail_address.zip
10 changes: 10 additions & 0 deletions app/views/component/detail/_service_areas.html.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
- if @service_areas.present?
%section{ class: 'service-areas' }
- if defined? stitle
- if stitle.present?
Copy link
Member

Choose a reason for hiding this comment

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

I don't think these two conditionals are necessary. Per my previous comment, the h1 should be read from the locales file, which will have a value by default.

%h1 #{stitle}:
%ul
- @service_areas.sort.each do |f|
%li
%span
= f
2 changes: 1 addition & 1 deletion app/views/component/detail/_template.html.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
- if org[field].present?
%section{:class=>css_class_format(field.to_s)}
%section{ class: css_class_format(field.to_s) }
- if defined? iconclass
%i{ class: iconclass }
- if defined? stitle
Expand Down
2 changes: 1 addition & 1 deletion app/views/component/detail/_urls.html.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
- if org[:urls].present?
%section{:class=>'urls'}
%section{ class: 'urls' }
- if defined? iconclass
%i{ class: iconclass }
- if defined? stitle
Expand Down
24 changes: 18 additions & 6 deletions app/views/component/organizations/detail/_body.html.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,14 +102,24 @@
%section#categories-box
%h1 Service Categories
%section.services{ itemscope: '', itemtype: 'http://schema.org/Service' }
%ul{ itemscope: '', itemtype: 'http://schema.org/ServiceType' }
%ul{ itemscope: '', itemtype: 'http://schema.org/serviceType' }
- @categories.each do |category|
%li{ class: "depth#{category.depth}" }
%li{ class: "hierarchy-depth-#{category.depth}" }
%span{ itemprop: 'serviceType' }
-# Category links are disabled till there's a corresponding filter that can be toggled!
= link_to category.name, organizations_path(@search_params.merge(:category => category.name))
= link_to category.name, organizations_path(@search_params.merge(category: category.name))
= category.name

- if @keywords.present?
%section#keywords-box
%h1 Keyword Tags
%section.keywords
%ul
- @keywords.each do |keyword|
%li
%span
= link_to keyword, organizations_path(@search_params.merge(keyword: keyword))

Copy link
Member

Choose a reason for hiding this comment

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

The keywords section should be moved to its own partial. This will make it easier to remove from _body if desired.

/ Main detail content.
%section

Expand All @@ -118,7 +128,7 @@
%section

= render partial: 'component/detail/template', locals: { org: @org, field: :short_desc }
= render partial: 'component/detail/template', locals: { org: @org, field: :description, stitle: 'Description' }
= render partial: 'component/detail/template', locals: { org: @org, field: :description, stitle: 'Location Description' }

- if @org[:coordinates].present? || @org[:transportation].present?
%section.map-box
Expand Down Expand Up @@ -167,14 +177,16 @@
%section.service{ itemscope: '', itemtype: 'http://schema.org/Service' }

= render partial: 'component/detail/template', locals: { org: service, field: :name}
= render partial: 'component/detail/template', locals: { org: service, field: :description}
= render partial: 'component/detail/template', locals: { org: service, field: :description, stitle: 'Service Description'}
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to stop using this pattern, please. This is one of the areas I'm planning on refactoring, so the existing code can be cleaned up later, but any new partials we add should adhere to a better pattern. The way the component/detail/template is set up, it behaves very much like a method that takes parameters sent from another partial. That kind of logic is best kept out of the view. If there is a need to DRY things up, that logic belongs in a helper method. However, care must be taken to not make things so DRY that they become cracked. Damp is better than cracked and hard to read and understand.

In the details view, each section represents a different field, and it's possible that one might want to style or mark up some fields differently. By using a template, it makes it harder to customize a particular field. The other problem with this approach is that every time the page is rendered, each field goes through the same logic, even though that logic will never apply to certain fields. It would be faster to render the non-array fields (like Audience, Eligibility, Fees, etc.) directly, than have the code go through a conditional first.

In this instance, the best practice would be to have a separate partial for each field, each with its own custom markup, like the languages partial. This way, there's only one place where you need to look to understand what a partial is rendering. With the existing template, there is too much thinking involved. You have to go to the _body partial, then make a note of the local variables that were sent to the _template partial, then keep track of those variables in your head while reading through the multiple conditionals, then think some more about whether or not the field is an Array, assuming you know off the top of your head what kind it is.

Also, any new content we add should make any user-facing text customizable. So, the h1 for these new fields should be added to config/locales/en.yml and referred to in the partial with the t() method. For the Service fields, I think a good name for the key would be service_fields. So, it would be something like this:

en:
  service_fields:
    eligibility: 'Eligibility'
    fees: 'Fees'

In the view, you would use t('service_fields.fees').
Please maintain alphabetical order within en.yml.
Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer to stop using this pattern, please. This is one of the areas I'm planning on refactoring, so the existing code can be cleaned up later, but any new partials we add should adhere to a better pattern. The way the component/detail/template is set up, it behaves very much like a method that takes parameters sent from another partial. That kind of logic is best kept out of the view. If there is a need to DRY things up, that logic belongs in a helper method.

It sounds like a helper method might be the way to go. From the CSS-side it's preferable to have these in a consistent HTML template, with permutations on the template easily identified. If they all have their own named partial it becomes difficult to know which partials need to be updated and which don't if the underlying shared HTML structure needs to change. I'd like to move more toward a Web Component model where UI components are a self-contained collection of HTML/CSS.

In the details view, each section represents a different field, and it's possible that one might want to style or mark up some fields differently.

Right now they each have a unique class name, so can be styled differently through that. The underlying mark up can be separated out for the ones that are different, as is done now, but it's preferable that the same HTML markup be shared.

By using a template, it makes it harder to customize a particular field.

Ideally, the sections would have underlying components defined through a CSS class and a shared HTML markup and if someone wanted to customized this they'd swap in a different component appearance by changing the class and possibly using a different underlying markup.

E.g. right now we have:

<section class="eligibility"><h1>Eligibility:</h1><span>None</span></section>

Instead we should probably add a component class to this, something like:

<section class="summary eligibility"><h1>Eligibility:</h1><span>None</span></section>

Then if someone wanted to customize this, they'd maybe have a different set of component styles they'd apply with:

<section class="summary-compact eligibility"><h1>Eligibility:</h1><span>None</span></section>

... that maybe was the summary but had less padding/margin and fit on one-line or something.

Additionally, they could do customizations to this specific component instance through the .eligibility class.

Anyway, I'm not sure what that looks like on the Rails side, but point is any re-usable UI chunk in the design should be isolated and have a shared HTML structure with other UI chunks that are the same.

Copy link
Member

Choose a reason for hiding this comment

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

Given that 1) the HTML structure has already been defined and is not likely to change, and 2) the current template is slowing down the view rendering, the simplest solution I would go with today is to have partials for each field, and you can make sure each partial has the correct HTML structure. Then, if it makes sense to refactor into helper methods, that can be explored. The result of refactoring should never be less efficient or less readable than the previous solution.

As an example, take a look at the "Edit location" form in the admin interface. It contains a bunch of fields, of which many share the same HTML structure. I could extract out the various types of similar fields like in this Railscasts, but I personally wouldn't go down that route. I think it's too much added code and complexity for little benefit.

Copy link
Member

Choose a reason for hiding this comment

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

Having said the above, this scenario is not as complicated as ActiveModel form objects, and it should be possible to create simple helper methods, but it's a little more involved than you might think.

There will need to be at least 2 helper methods: one for array fields, and one for text fields. The problem with your current template is it tries to be a one-size-fits-all solution, and breaks the Tell, Don't Ask principle.

In your template, every field gets asked the same question before it can be rendered: "Are you an array?" For array fields, the answer will always be "Yes", and for text fields, "No". If you already know the answer beforehand, why waste time asking the question? Instead, you should tell the array fields to do one thing, and text fields to do another thing by creating separate methods for them.

Because the methods act on an object, you will need to pass the Service object as a parameter, along with other parameters, like the field name and h1. However, creating a regular helper module with methods that could be used with any object (not just a Service), would be wrong since we'd like to use an object oriented approach.

This means we'll need to create a separate class, called ServicePresenter for example, which will be instantiated in the _body partial like this for each service: service_presenter = ServicePresenter.new(service), then you can call the class method like this: service_presenter.array_field(:service_areas, t('service_fields.service_areas').

= render partial: 'component/detail/urls', locals: { org: service, field: :urls, stitle: 'Homepage' }
= render partial: 'component/detail/template', locals: { org: service, field: :fees, stitle: 'Fees' }
= render partial: 'component/detail/template', locals: { org: service, field: :audience, stitle: 'Audience' }
= render partial: 'component/detail/template', locals: { org: service, field: :eligibility, stitle: 'Eligibility' }
= render partial: 'component/detail/template', locals: { org: service, field: :how_to_apply, stitle: 'How to Apply' }
= render partial: 'component/detail/template', locals: { org: service, field: :wait, stitle: 'Service Wait Estimate' }
= render partial: 'component/detail/service_areas', locals: { org: service, field: :service_areas, stitle: 'Service Areas' }

/ detail footer content
/ Detail footer content.
- if @org[:updated_at].present?
%footer
%section.metadata
Expand Down
60 changes: 31 additions & 29 deletions spec/features/details/location_details_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,19 +92,12 @@
expect(page).to have_content('Walk in or apply by phone or mail')
end

# Wait time not included in the view.
# Till we have a consistent meaning for wait time
# it's best left out of the front-end view
xit 'includes wait info' do
it 'includes wait estimate info' do
expect(page).to have_content('No wait to 2 weeks')
end

# Service areas not included in view.
# Best to leave this out of the view, this is data that could easily be
# wrong and it's better that the client contact the agency and ask for
# services and be referred accordingly vs. going off this list.
xit 'includes service areas' do
expect(page).to have_content('Marin County')
it 'includes service areas info' do
expect(page).to have_content('San Mateo County')
end

it 'includes updated time' do
Expand All @@ -121,8 +114,27 @@
end
end

it 'includes URLs' do
expect(page).to have_link('www.smchealth.org')
it 'includes short description' do
within '.short-desc' do
expect(page).
to have_content('[NOTE THIS IS NOT A REAL ENTRY--')
end
end

it 'includes description' do
expect(page).to have_content('Lorem ipsum')
end

it 'includes transporation info' do
expect(page).to have_content('SAMTRANS stops within 1 block')
end

it 'includes hours info' do
expect(page).to have_content('Monday-Friday, 8-5; Saturday, 10-6;')
end

it 'includes languages info' do
expect(page).to have_content('Chinese (Cantonese)')
end

it 'includes accessibility info' do
Expand All @@ -138,36 +150,26 @@
expect(page).to have_link('sanmaceo@co.sanmaceo.ca.us')
end

it 'includes hours info' do
expect(page).to have_content('Monday-Friday, 8-5; Saturday, 10-6;')
it 'includes URLs' do
expect(page).to have_link('www.smchealth.org')
end

it 'includes languages info' do
expect(page).to have_content('Chinese (Cantonese)')
it 'includes Physical Address info' do
expect(page).to have_content('Avenue of the fellows')
end

it 'includes Mailing Address info' do
expect(page).to have_content('Hella Fellas')
end

it 'includes description' do
expect(page).to have_content('Lorem ipsum')
end

it 'includes short description' do
within '.short-desc' do
expect(page).
to have_content('[NOTE THIS IS NOT A REAL ENTRY--')
end
end

it 'includes transporation info' do
expect(page).to have_content('SAMTRANS stops within 1 block')
it 'includes keywords' do
expect(page).to have_link('Ruby on Rails/MongoDB/Redis')
end

it 'sets the page title to the location name + site title' do
expect(page).to have_title('San Maceo Agency | Ohana Web Search')
end

end

describe 'Edit link' do
Expand Down