Zmienne lokalne w widokach cz.3

Zakład patologii kodu
1

fc to FaqCategory (has_many :faq_items)

[code=ruby]app/views/faqs/index.html.erb

<% @faq_categories.each { |fc| %> <%= render :partial => 'faq_item', :locals => { :fc => fc } %> <% } %>

app/views/faqs/faq_item.html.erb
<%
content = ‘’
unless fc.faq_items.blank?
fc.faq_items.each { |fi|
content << content_tag(:p, link_to(fi.question, “javascript:void(0)”, :onclick => "new Effect.toggle($('faq_item#{fi.id}’),‘blind’, {duration: 0.2})"))
content << content_tag(:div, fi.answer, :class => ‘faq_item’, :id => “faq_item_#{fi.id}”, :style => “display: none”)
}
end
%>

<% unless fc.faq_items.blank? %>
<%= content_tag(:p, link_to(“Expand”, “javascript:void(0)”, :onclick => “new Effect.toggle($(‘faq_category_#{fc.id}’),‘blind’, {duration: 0.2, afterFinish: function() { toggleLink($(‘faq_category_#{fc.id}’), $('faq_category_#{fc.id}toggle’)); }})", :id => "faq_category#{fc.id}toggle"), :class => “link”) %>
<%= content_tag(:h3, fc.name) %>
<%= content_tag(:div, content, :class => ‘faq_category’, :id => "faq_category#{fc.id}”, :style => “display: none;”) %>


<% end %>[/code]
Wszelkie sugestie jak poprawić powyższy kod mile widziane.
2

Wklejam jeszcze kod funkcji toggleLink (js) :

function toggleLink(itemId, toggleId) { if (itemId.style.display == "none") { toggleId.innerHTML = "Expand"; } else { toggleId.innerHTML = "Close"; } }

3

Akurat poprawiam ten kod więc wrzucam moje poprawki też tutaj:

Na początek można pozbyć się zmiennej lokalnej content i podwójnego wołania unless fc.faq_items.blank?
Programista który to pisał nie wiedział chyba, że content_tag może przyjmować blok (przydaje się przy bardziej złożonej strukturze, np. tworzymy taga i w nim zagnieżdżamy kilka innych tagów).
Poza tym relacja has_many domyślnie zwraca pustą tablicę a nie nil więc używanie blank? działa ale jest zbyteczne, lepiej skorzystać z empty?

[code=ruby]<% unless fc.faqs.empty? -%>
<%= content_tag(:p, link_to(“Expand”, “javascript:void(0)”, :onclick => “new Effect.toggle($(‘faq_category_#{fc.id}’),‘blind’, {duration: 0.2, afterFinish: function() { toggleLink($(‘faq_category_#{fc.id}’), $('faq_category_#{fc.id}toggle’)); }})", :id => "faq_category#{fc.id}toggle"), :class => “link”) %>
<%= content_tag(:h3, fc.title) %>
<% content_tag(:div, :class => ‘faq_category’, :id => "faq_category#{fc.id}”, :style => “display: none;”) do %>
<% fc.faqs.each do |fi| -%>
<%= content_tag(:p, link_to(fi.question, “javascript:void(0)”, :onclick => “new Effect.toggle($(‘faq_item_#{fi.id}’),‘blind’, {duration: 0.2})”)) -%>
<%= content_tag(:div, fi.answer, :class => ‘faq_item’, :id => “faq_item_#{fi.id}”, :style => “display: none”) -%>
<% end -%>
<% end -%>

<% end -%>[/code]
4

Jest lepiej ale może być jeszcze lepiej :slight_smile:

Ręczne składanie unikalnych id obiektów (które są wykorzystywane w kodzie js) zastępujemy helperem dom_id
Kod działa zasadniczo tak samo, tylko jest trochę bardziej przejrzysty:

[code=ruby]<% unless fc.faqs.empty? -%>
<%= content_tag(:p, link_to(“Expand”, “javascript:void(0)”, :onclick => “new Effect.toggle($(’#{dom_id(fc)}’),‘blind’, {duration: 0.2, afterFinish: function() { toggleLink($(’#{dom_id(fc)}’), $(’#{dom_id(fc)}_toggle’)); }})”, :id => “#{dom_id(fc)}_toggle”), :class => “link”) %>
<%= content_tag(:h3, fc.title) %>
<% content_tag(:div, :class => ‘faq_category’, :id => “#{dom_id(fc)}”, :style => “display: none;”) do %>
<% fc.faqs.each do |fi| -%>
<%= content_tag(:p, link_to(fi.question, “javascript:void(0)”, :onclick => “new Effect.toggle($(’#{dom_id(fi)}’),‘blind’, {duration: 0.2})”)) -%>
<%= content_tag(:div, fi.answer, :class => ‘faq_item’, :id => “#{dom_id(fi)}”, :style => “display: none”) -%>
<% end -%>
<% end -%>

<% end -%>[/code]
5

Czy może być jeszcze lepiej ? Może :wink:

Pozbywamy się tych wszystkich unikalnych id, elementów wyszukujemy przy pomocy klas CSS oraz ich relatywnego położenia.
Kod inline js zastępujemy kodem “unobtrusive” a Prototype zastępujemy jQuery:

[code=ruby]<% content_for(:head) do -%>
<%= javascript_include_tag(“faqs”) -%>
<% end -%>

<% unless fc.faqs.empty? -%>
<%= content_tag(:p, link_to(“Expand”, “#nogo”, :class => “category_link”)) %>
<%= content_tag(:h3, fc.title) %>
<% content_tag(:div, :class => ‘faq_category’, :style => “display: none”) do %>
<% fc.faqs.each do |fi| -%>
<%= content_tag(:p, link_to(fi.question, “#nogo”, :class => “faq_link”)) -%>
<%= content_tag(:div, fi.answer, :class => ‘faq_answer’, :style => “display: none”) -%>
<% end -%>
<% end -%>

<% end -%>[/code] [code=javascript]public/javascripts/faqs.js

$(document).ready(function () {
$(‘a.category_link’).click(function() {
$(this).parent().nextAll(’.faq_category’).slideToggle(200);
if ($(this).text() == “Expand”) {
$(this).text(“Close”);
} else {
$(this).text(“Expand”);
}
});
$(‘a.faq_link’).click(function() {
$(this).parent().next().slideToggle(200);
});

});[/code]

6

Nie jestem świetnym znawcą Javascript/jQuery więc jeśli ktoś ma jakieś poprawki do powyższego kodu to chętnie je zobaczę. Dzięki!

7

Ja bym się jeszcze pozbył tych wszystkich content_tagów.

8

@hosiawak - zainteresuj się funkcją toggle api :wink:

9

Czy coś takiego masz na myśli:

[code=javascript]$(document).ready(function () {
$(‘a.category_link’).toggle(function() {
$(this).parent().nextAll(’.faq_category’).first().show();
$(this).text(“Close”);
},
function() {
$(this).parent().nextAll(’.faq_category’).first().hide();
$(this).text(“Expand”);
});
$(‘a.faq_link’).click(function() {
$(this).parent().next().toggle();
});

});[/code]
Przy okazji zapytam: dlaczego nie działa $(this).parent().next(’.faq_category’).show() - muszę użyć nextAll() ?

10

To pokazuje doskonale, dlaczego ERb sucks :slight_smile: W hamlu:

- unless fc.faqs.empty? %p.category_link= link_to "Expand", "#nogo" %h3= fc.title .faq_category{:style => "display: none"} - fc.faqs.each do |fi| %p.faq_link= link_to fi.question, "#nogo" .faq_answer{:style => "display: none"}= fi.answer .clear
Zrobiłem dwie drobne zmiany - styl przyczepiłem do tagu “p”.

EDIT

No i jeszcze uwaga odnośnie nazewnictwa (u mnie to już tradycja :slight_smile: - bezsensowne jest przekazywanie zmiennej lokalnej “fc” (Barcelona?) do tego “parszala”.
Zdecydowanie lepiej zrobić = render :partial => "category_item", :object => fc
i w “parszalu” odwoływać się do “category_item”.
Tym bardziej jest to mylące, kiedy do “_faq_item” przekazywanay jest obiekt “fc” (FaqCategory) (dlatego właśnie zmieniłem tę nazwę “parszala”).