Custom page part title #2875

bricesanchez · 2015-01-30T05:50:47Z

It's a working progress. This feature works but tests are failing on my computer.

I create this PR to share my progress and ask for inputs. I don't know if i'm doing the right things on controllers and models.

What do you think about that ?

simi · 2015-01-30T13:04:03Z

core/app/assets/javascripts/refinery/admin.js.erb

@@ -591,15 +598,23 @@ var page_options = {
      $('#new_page_part_dialog').dialog('open');
    });

+    $('#new_page_part_title').on('keyup', function() {


This should be on "input" event. But it doesn't work for IE8 and IE9. But alternatively "propertychange" event can be used there.

bricesanchez · 2015-03-14T06:06:26Z

Hi @parndt,

I've updated this PR, could you review it and help me to understand why i've tests who fails?

parndt · 2015-03-14T06:33:49Z

@bricesanchez because slug is required on PagePart but the test isn't setting it.. shouldn't the slug be able to default to title.to_param?

parndt · 2015-03-14T06:35:21Z

Here's the validator and here's where it's needed

bricesanchez · 2015-03-14T21:45:30Z

Thanks @parndt, it works now!

shouldn't the slug be able to default to title.to_param?

it probably should but how ?

parndt · 2015-03-15T04:08:34Z

Something like this:

class PagePart
  def slug
    super || title.to_s.to_param
  end
end

bricesanchez · 2015-03-16T02:58:34Z

Yes, i would like to use underscores instead of dashes.

My method looks like this now :

class PagePart
  def slug
    super || title.to_s.parameterize
  end
end

But with this i'm able to save a record with a title but a nil slug

parndt · 2015-03-16T03:03:01Z

That's weird, is the title blank?

bricesanchez · 2015-03-16T03:04:10Z

Nope :

<Refinery::PagePart id: 10, refinery_page_id: nil, slug: nil, body: nil, position: nil, created_at: "2015-03-16 02:48:58", updated_at: "2015-03-16 02:48:58", title: "er rew e 24 ^">

parndt · 2015-03-16T03:04:39Z

What about

class PagePart
  def slug
    super.presence || title.to_s.parameterize.underscore.presence
  end
end

bricesanchez · 2015-03-16T03:06:25Z

irb(main):001:0> p = Refinery::PagePart.new
=> #<Refinery::PagePart id: nil, refinery_page_id: nil, slug: nil, body: nil, position: nil, created_at: nil, updated_at: nil, title: nil>
irb(main):002:0> p.title = "esfefse f33\U+FFC3\U+FFA8"
=> "esfefse f33"
irb(main):003:0> p.save
   (0.1ms)  begin transaction
  Refinery::PagePart Exists (0.1ms)  SELECT  1 AS one FROM "refinery_page_parts"  WHERE ("refinery_page_parts"."slug" = 'esfefse_f33' AND "refinery_page_parts"."refinery_page_id" IS NULL) LIMIT 1
  SQL (0.2ms)  INSERT INTO "refinery_page_parts" ("created_at", "title", "updated_at") VALUES (?, ?, ?)  [["created_at", "2015-03-16 03:05:28.025965"], ["title", "esfefse f33"], ["updated_at", "2015-03-16 03:05:28.025965"]]
   (4.4ms)  commit transaction
=> true
irb(main):004:0> p = Refinery::PagePart.last
  Refinery::PagePart Load (0.1ms)  SELECT  "refinery_page_parts".* FROM "refinery_page_parts"   ORDER BY "refinery_page_parts"."id" DESC LIMIT 1
  Refinery::PagePart::Translation Load (0.1ms)  SELECT "refinery_page_part_translations".* FROM "refinery_page_part_translations"  WHERE "refinery_page_part_translations"."refinery_page_part_id" = ?  [["refinery_page_part_id", 11]]
=> #<Refinery::PagePart id: 11, refinery_page_id: nil, slug: nil, body: nil, position: nil, created_at: "2015-03-16 03:05:28", updated_at: "2015-03-16 03:05:28", title: "esfefse f33">

parndt · 2015-03-16T03:06:45Z

Or probably more accurately:

class PagePart
  before_validation :set_default_slug

  private
  def set_default_slug
    self.slug = title.to_s.parameterize.underscore.presence if self.slug.blank?
  end
end

bricesanchez · 2015-03-16T03:12:31Z

Now it works well and i understand the syntax 😄

parndt · 2015-03-16T23:51:27Z

pages/app/models/refinery/page.rb

@@ -312,11 +312,11 @@ def content_for?(part_title)
    #    ::Refinery::Page.first.part_with_title(:body)
    #
    # Will return the Refinery::PagePart object with that title using the first page.
-    def part_with_title(part_title)
+    def part_with_slug(part_slug)


Can you please deprecate the old API, and update the documentation above this method to be correct?
Same thing needs to happen for content_for, and content_for?

parndt · 2015-03-16T23:55:53Z

Normally we handle deprecations of public methods with the Refinery.deprecate method. You should find examples in the codebase already for things that are changing from 2.1 -> 3.0

It also means you're going to have to handle the old API calls until e.g. version 3.1 so for part_with_title that would look something like this (so that it actually calls the new API underneath with the right argument):

def part_with_title(part_title)
  Refinery.deprecate("Refinery::Page#part_with_title", when: "3.1", replacement: "part_with_slug")
  part_with_slug(part_title.to_s.parameterize.underscore)
end

Understand what I mean?

bricesanchez · 2015-03-17T14:29:35Z

Yep ! i will do that ASAP. 😄

bricesanchez · 2015-04-16T02:09:16Z

parndt · 2015-04-16T02:13:14Z

@simi are you happy with us maintaining this feature for the rest of time?

simi · 2015-04-16T12:34:31Z

Yes, I'm happy to maintain this for the rest of my poor life. But do we really need https://github.com/slav123/jquery-seo-url/blob/master/jquery.seourl.js included? If yes, why minificated?

bricesanchez · 2015-04-17T13:41:36Z

Seo url support all accents like (éèàçùî....), i've changed it for the unminificated version

simi · 2015-04-17T13:53:40Z

core/app/assets/javascripts/refinery/jquery.seourl.js

+
+(function( $ ){
+
+  // protorype, and return string


prototype (patch sent - slav123/jquery-seo-url#1)

simi · 2015-04-17T23:00:21Z

It is released as https://rubygems.org/gems/speakingurl-rails/versions/1.1.2 now.

bricesanchez · 2015-04-21T03:41:57Z

I've made the changes :)

simi · 2015-04-21T13:51:25Z

pages/app/models/refinery/page_part.rb

@@ -34,11 +42,16 @@ def normalise_text_fields

    private
    def parameterize(string)
+


i will cut that, thanks.

simi · 2015-04-21T13:52:50Z

What about to use new hash syntax?

bricesanchez · 2015-04-21T15:56:02Z

@simi, it will be great to use new hash syntax but may be in another refactor PR.

parndt · 2015-04-28T01:42:39Z

pages/app/views/refinery/admin/pages/_form_new_page_parts.html.erb

@@ -10,3 +14,7 @@
             :cancel_button_id => "new_page_part_cancel",
             :hide_delete => true %>
 </div>
+
+<% content_for :after_javascript_libraries do %>
+  <%= javascript_include_tag 'refinery/jquery.seourl' %>


Where does this file come from? I don't see it in the changeset.

Sorry i've forgot to remove it since we doesn't use seourl, replaced by speaking-url

Do we need to add this dependency to refinerycms-core? Shouldn't it be a dependency of refinerycms-pages?

good point, i move it to pages

parndt · 2015-04-28T04:25:36Z

pages/app/controllers/refinery/admin/page_parts_controller.rb

@@ -4,7 +4,7 @@ class PagePartsController < ::Refinery::AdminController

      def new
        render :partial => '/refinery/admin/pages/page_part_field', :locals => {
-                 :part => ::Refinery::PagePart.new(:title => params[:title], :body => params[:body]),
+                 :part => ::Refinery::PagePart.new(:title => params[:title], :slug => params[:slug], :body => params[:body]),


I think now this could just be:

:part => ::Refinery::PagePart.new(params.slice(:title, :slug, :body)),

simi · 2015-04-28T11:45:03Z

core/app/assets/javascripts/refinery/admin.js.erb

@@ -577,13 +577,17 @@ var page_options = {
  },

  page_part_dialog: function(){
+    var slugOptions = {
+      separator: '_'


Isn't - nicer?

Agree. If nothing else, Google recommends hyphens over underscores. It used to be better for SEO although I'm not sure that's still the case.

Plus, it's marginally easier to type, matches domain restrictions, so it looks more URL-y (underscores are not allowed in domain names.)

Sorry, i'm not agree.
We don't speak about the page slug but the page part slug.

The slug for the page part is never displayed as an entire page so Google will never see it.

When we call a page part in an html view, we write something like that :
<%= raw @page.content_for(:side_body) %>
not
<%= raw @page.content_for(:"side-body") %>

Yeah this is for programmatical reference, nothing shown anywhere. I agree with @bricesanchez

@parndt

Fix specs with slug support Use params[:slug] for page_part_field render Validate presence of title in PagePart model Add slug locales in FR and EN Fix page_part_dialog JS Change new_page_part_save errors text Fix visual_editor_add_image specs with the adding of :slug param (thanks @parndt) slug should be able to default to title.to_param (thanks @parndt) Set default slug before validation Deprecate old api for title_matches? and part_with_title methods Add feature and deprecations in changelog Add speakingurl-rails gem Rewrite page part title to slug using speakingurl Credit @simi for page part title feature Move speakingurl dependency to pages Precompile speakingurl only if new page part config is enable Q&A of @parndt fix speaking url load fix permitted params for page parts

Custom page part title

parndt · 2015-07-18T04:52:04Z

Thanks @bricesanchez

bricesanchez mentioned this pull request Jan 30, 2015

[Feature request] Custom/Translate page part title #2873

Closed

simi reviewed Jan 30, 2015
View reviewed changes

parndt reviewed Mar 16, 2015
View reviewed changes

bricesanchez self-assigned this Apr 1, 2015

bricesanchez added this to the 3.0 milestone Apr 1, 2015

bricesanchez force-pushed the page-part-title branch 2 times, most recently from 2853d62 to 83cc38f Compare April 15, 2015 03:06

bricesanchez force-pushed the page-part-title branch from 83cc38f to 5b526f7 Compare April 17, 2015 13:28

simi reviewed Apr 17, 2015
View reviewed changes

bricesanchez force-pushed the page-part-title branch from 5b526f7 to c7e9f39 Compare April 21, 2015 03:37

simi reviewed Apr 21, 2015
View reviewed changes

bricesanchez force-pushed the page-part-title branch from c7e9f39 to ca71f0f Compare April 22, 2015 01:17

parndt reviewed Apr 28, 2015
View reviewed changes

bricesanchez force-pushed the page-part-title branch from ca71f0f to 1c60cfa Compare April 28, 2015 04:23

parndt reviewed Apr 28, 2015
View reviewed changes

bricesanchez force-pushed the page-part-title branch from 1c60cfa to 024915d Compare April 28, 2015 04:28

simi reviewed Apr 28, 2015
View reviewed changes

bricesanchez force-pushed the page-part-title branch 2 times, most recently from 00f84b9 to 040300c Compare May 2, 2015 04:31

bricesanchez force-pushed the page-part-title branch from 9684bfd to 47ef396 Compare July 18, 2015 04:13

bricesanchez added 2 commits July 18, 2015 00:40

Remove useless rescue in content_for Page method

1c6d736

bricesanchez force-pushed the page-part-title branch from 47ef396 to 1c6d736 Compare July 18, 2015 04:40

parndt added a commit that referenced this pull request Jul 18, 2015

Merge pull request #2875 from bricesanchez/page-part-title

6396c1a

Custom page part title

parndt merged commit 6396c1a into refinery:master Jul 18, 2015

parndt mentioned this pull request Jul 18, 2015

New revised user experience - Glass #2999

Closed

bricesanchez mentioned this pull request Jul 20, 2015

Page parts title missing on newly updated website #3010

Closed

		@@ -34,11 +42,16 @@ def normalise_text_fields

		private
		def parameterize(string)

Custom page part title #2875

Custom page part title #2875

Conversation

bricesanchez commented Jan 30, 2015

Choose a reason for hiding this comment

bricesanchez commented Mar 14, 2015

parndt commented Mar 14, 2015

parndt commented Mar 14, 2015

bricesanchez commented Mar 14, 2015

parndt commented Mar 15, 2015

bricesanchez commented Mar 16, 2015

parndt commented Mar 16, 2015

bricesanchez commented Mar 16, 2015

parndt commented Mar 16, 2015

bricesanchez commented Mar 16, 2015

parndt commented Mar 16, 2015

bricesanchez commented Mar 16, 2015

Choose a reason for hiding this comment

parndt commented Mar 16, 2015

bricesanchez commented Mar 17, 2015

bricesanchez commented Apr 16, 2015

parndt commented Apr 16, 2015

simi commented Apr 16, 2015

bricesanchez commented Apr 17, 2015

Choose a reason for hiding this comment

simi commented Apr 17, 2015

bricesanchez commented Apr 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simi commented Apr 21, 2015

bricesanchez commented Apr 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parndt commented Jul 18, 2015