Merge pull request #2368 from tvdeyen/deprecate-element-dom-id

Use position for element dom_id

app/helpers/alchemy/elements_block_helper.rb

@@ -137,7 +137,7 @@ def ingredient_by_role(role)
     def element_view_for(element, options = {})
       options = {
         tag: :div,
-        id: element_dom_id(element),
+        id: element.dom_id,
         class: element.name,
         tags_formatter: ->(tags) { tags.join(" ") },
       }.merge(options)

app/helpers/alchemy/elements_helper.rb

@@ -167,12 +167,13 @@ def render_element(element, options = {}, counter = 1)
     end
 
     # Returns a string for the id attribute of a html element for the given element
+    # @deprecated
     def element_dom_id(element)
-      return "" if element.nil?
-
-      "#{element.name}_#{element.id}".html_safe
+      element&.dom_id
     end
 
+    deprecate element_dom_id: "element.dom_id", deprecator: Alchemy::Deprecation
+
     # Renders the HTML tag attributes required for preview mode.
     def element_preview_code(element)
       tag_builder.tag_options(element_preview_code_attributes(element))

app/helpers/alchemy/url_helper.rb

@@ -35,8 +35,11 @@ def download_alchemy_attachment_url(attachment)
     end
 
     # Returns the full url containing host, page and anchor for the given element
+    # @deprecated
     def full_url_for_element(element)
-      "#{current_server}/#{element.page.urlname}##{element_dom_id(element)}"
+      "#{current_server}/#{element.page.urlname}##{element.dom_id}"
     end
+
+    deprecate :full_url_for_element, deprecator: Alchemy::Deprecation
   end
 end

app/models/alchemy/element/presenters.rb

@@ -82,7 +82,7 @@ def display_name_with_preview_text(maxlength = 30)
       # Returns a dom id used for elements html id tag.
       #
       def dom_id
-        "#{name}_#{id}"
+        [parent_element&.dom_id, name, position].compact.join("-")
       end
 
       # The content that's used for element's preview text.

app/views/alchemy/pages/show.rss.builder

@@ -13,8 +13,8 @@ xml.rss version: "2.0" do
         if element.has_ingredient?("date")
           xml.pubDate element.ingredient("date").to_s(:rfc822)
         end
-        xml.link show_alchemy_page_url(@page, anchor: element_dom_id(element))
-        xml.guid show_alchemy_page_url(@page, anchor: element_dom_id(element))
+        xml.link show_alchemy_page_url(@page, anchor: "##{element.dom_id}")
+        xml.guid show_alchemy_page_url(@page, anchor: "##{element.dom_id}")
       end
     end
   end

spec/helpers/alchemy/elements_block_helper_spec.rb

@@ -8,7 +8,7 @@ module Alchemy
   describe "ElementsBlockHelper" do
     let(:page) { create(:alchemy_page, :public) }
     let(:element) { create(:alchemy_element, page: page, tag_list: "foo, bar") }
-    let(:expected_wrapper_tag) { "div.#{element.name}##{element_dom_id(element)}" }
+    let(:expected_wrapper_tag) { "div.#{element.name}##{element.dom_id}" }
 
     describe "#element_view_for" do
       it "should yield an instance of ElementViewHelper" do

spec/helpers/alchemy/elements_helper_spec.rb

@@ -27,7 +27,7 @@ module Alchemy
         end
 
         it "renders the element's view partial" do
-          is_expected.to have_selector("##{element.name}_#{element.id}")
+          is_expected.to have_selector("##{element.dom_id}")
         end
 
         context "with element view partial not found" do
@@ -65,10 +65,16 @@ module Alchemy
     end
 
     describe "#element_dom_id" do
+      around do |example|
+        Alchemy::Deprecation.silence do
+          example.run
+        end
+      end
+
       subject { helper.element_dom_id(element) }
 
       it "should render a unique dom id for element" do
-        is_expected.to eq("#{element.name}_#{element.id}")
+        is_expected.to eq(element.dom_id)
       end
     end
 
@@ -83,8 +89,8 @@ module Alchemy
         let(:options) { {} }
 
         it "should render all elements from current pages public version." do
-          is_expected.to have_selector("##{element.name}_#{element.id}")
-          is_expected.to have_selector("##{another_element.name}_#{another_element.id}")
+          is_expected.to have_selector("##{element.dom_id}")
+          is_expected.to have_selector("##{another_element.dom_id}")
         end
 
         context "in preview_mode" do
@@ -95,7 +101,7 @@ module Alchemy
           end
 
           it "page draft version is used" do
-            is_expected.to have_selector("##{draft_element.name}_#{draft_element.id}")
+            is_expected.to have_selector("##{draft_element.dom_id}")
           end
         end
       end
@@ -125,8 +131,8 @@ module Alchemy
           let!(:another_element) { create(:alchemy_element, page: another_page, page_version: another_page.public_version) }
 
           it "should render all elements from that page." do
-            is_expected.to have_selector("##{element.name}_#{element.id}")
-            is_expected.to have_selector("##{another_element.name}_#{another_element.id}")
+            is_expected.to have_selector("##{element.dom_id}")
+            is_expected.to have_selector("##{another_element.dom_id}")
           end
 
           context "in preview_mode" do
@@ -137,7 +143,7 @@ module Alchemy
             end
 
             it "page draft version is used" do
-              is_expected.to have_selector("##{draft_element.name}_#{draft_element.id}")
+              is_expected.to have_selector("##{draft_element.dom_id}")
             end
           end
         end
@@ -165,7 +171,7 @@ module Alchemy
         end
 
         it "uses that to load elements to render" do
-          is_expected.to have_selector("#news_1001")
+          is_expected.to have_selector("#news-1")
         end
       end
 

spec/helpers/alchemy/url_helper_spec.rb

@@ -141,6 +141,12 @@ module Alchemy
     end
 
     describe "#full_url_for_element" do
+      around do |example|
+        Alchemy::Deprecation.silence do
+          example.run
+        end
+      end
+
       subject { full_url_for_element(element) }
 
       let(:element) { create(:alchemy_element, name: "headline") }

spec/models/alchemy/element_spec.rb

@@ -541,10 +541,24 @@ module Alchemy
     end
 
     describe "#dom_id" do
-      let(:element) { build_stubbed(:alchemy_element) }
+      let(:element) { build_stubbed(:alchemy_element, position: 1) }
+
+      it "returns a string from element name and position" do
+        expect(element.dom_id).to eq("#{element.name}-#{element.position}")
+      end
+
+      context "with a parent element" do
+        let(:parent_element) do
+          build_stubbed(:alchemy_element, position: 1)
+        end
 
-      it "returns an string from element name and id" do
-        expect(element.dom_id).to eq("#{element.name}_#{element.id}")
+        let(:element) do
+          build_stubbed(:alchemy_element, position: 1, parent_element: parent_element)
+        end
+
+        it "returns a string from element name and position" do
+          expect(element.dom_id).to eq("#{parent_element.name}-#{parent_element.position}-#{element.name}-#{element.position}")
+        end
       end
     end
 

spec/support/custom_news_elements_finder.rb

@@ -2,6 +2,6 @@
 
 class CustomNewsElementsFinder
   def elements(*)
-    [Alchemy::Element.new(name: "news", id: 1001)]
+    [Alchemy::Element.new(name: "news", id: 1001, position: 1)]
   end
 end