Add toc_pages plugin by okkez · Pull Request #691 · bm-sms/daimon-news

okkez · 2017-01-12T05:16:55Z

This is a daimon_markdown plugin.

mtsmfm

Great work!
But TableOfContentsPages#call method is difficult and complex for me 😢

mtsmfm · 2017-01-17T08:18:32Z

lib/daimon_markdown/plugins/toc_pages.rb

+
+        request = context[:request]
+
+        current_page = if request.params.key?(:page)


How about passing context expressly (:page, :public_id etc) instead of :request?
It is difficult for me to find what params this plugin needs 😕

I want to use full functionalities of request in this plugin.
But in fact, this plugin uses some functionalities of request.

okkez · 2017-01-26T08:31:49Z

I've fixed and pushed changes.
@mtsmfm Could you check this PR again?

mtsmfm · 2017-01-27T09:50:17Z

app/controllers/editor/posts_controller.rb


+    @context = {
+      full_text: @post.body,
+      fullpath: request.fullpath,


Hmm, I want to have uniform name
(fulltext and fullpath) or (full_text and full_path)
but fulltextdoesn't make sence...

mtsmfm · 2017-01-27T09:52:16Z

app/controllers/editor/posts_controller.rb

        @post.pages.page(params[:page]).per(1)
      end

+    @context = {


How about markdown_context ?
markdown_context is straightforward I think.

@context

@context -> @markdown_context It's straightforward.

Because `full_text` and `fullpath` don't have same form. It's very confusing us.

okkez-bot · 2017-02-23T01:14:08Z

test/lib/daimon_markdown/toc_pages_test.rb

+    end
+  end
+
+  sub_test_case "toc" do


[rubocop]

Metrics/BlockLength: Block has too many lines. [114/25]

okkez-bot · 2017-02-23T01:14:08Z

test/lib/daimon_markdown/toc_pages_test.rb

+    data("page1" => [1, 0, ["title"]],
+         "page2" => [2, 1, ["title-2"]],
+         "page3" => [3, 2, ["title-3", "title-3-1", "title-3-2"]])
+    test "multiple pages with complex source" do |(page, index, expected_header_ids)|


[rubocop]

Metrics/BlockLength: Block has too many lines. [36/25]

mtsmfm · 2017-02-23T03:02:38Z

lib/daimon_markdown/plugins/toc_pages.rb

+        page_separated_nodes = []
+        condition = 1.upto(6).map {|n| "./following::h#{n}" }.join("|")
+        html.search("//comment()[contains(.,\"nextpage\")]").map do |comment_element|
+          page_separated_nodes << comment_element.search(condition).first


page_separated_nodes = html.search("//comment()[contains(.,\"nextpage\")]").map do |comment_element| comment_element.search(condition).first end

Ah! Good catch!

mtsmfm · 2017-02-23T03:06:46Z

lib/daimon_markdown/plugins/toc_pages.rb

+            end
+          headers[id] += 1
+          header_content = header_node.children.first
+          # TODO: Arrange indent level


This is for generated html indentation.
It is not important.

`#at` returns first element which is matched condition.

okkez-bot · 2017-02-23T09:54:27Z

lib/daimon_markdown/plugins/toc_pages.rb

+        headers_per_page = Hash.new {|h, k| h[k] = [] }
+        ul_list = Hash.new {|h, k| h[k] = UnorderedList.new }
+        ul_list[1] = UnorderedList.new(html_class: toc_class)
+        # rubocop:disable Metrics/BlockLength


[rubocop]

Lint/UnneededDisable: Unnecessary disabling of Metrics/BlockLength.

okkez-bot · 2017-02-23T09:54:28Z

lib/daimon_markdown/plugins/toc_pages.rb

+
+        def to_html
+          if @items.empty?
+            %Q(<li>#{@header.link}</li>)


[rubocop]

Style/UnneededPercentQ: Use %Q only for strings that contain both single quotes and double quotes, or for dynamic strings that contain double quotes. :ref

ul must not be child of ul directry.

okkez-bot · 2017-02-24T06:19:51Z

lib/daimon_markdown/plugins/toc_pages.rb

+            (previous_level + 1).upto(header.level) do |level|
+              ul_list[level - 1] << ul_list[level]
+            end
+          else


[rubocop]

Style/EmptyElse: Redundant else-clause.

mtsmfm · 2017-02-28T01:26:53Z

I got 500 error when I previewed a post has following body:

{{toc_pages}}

# Hello
hi !

<!--nextpage -->

## h2

a

### h3

b

okkez · 2017-02-28T01:56:26Z

ActionView::Template::Error (undefined method `[]' for nil:NilClass):
  24:           = render 'posts/credits', credits: @post.credits.with_ordered
  25:       %p.thumbnail= image_tag @post.thumbnail_url
  26:     .post__body
  27:       = render_markdown(@pages.map(&:body).join, @markdown_context)
  28: 
  29:     - if @pages.total_pages > 1
  30:       %nav.pagination
lib/daimon_markdown/plugins/toc_pages.rb:57:in `block in call'
lib/daimon_markdown/plugins/toc_pages.rb:56:in `call'
lib/daimon_markdown/plugins/toc_pages.rb:56:in `zip'
app/helpers/markdown_helper.rb:4:in `render_markdown'
app/views/posts/show.html.haml:27:in `block in _app_views_posts_show_html_haml___3001049241409430540_70268641750820'
app/views/posts/show.html.haml:13:in `_app_views_posts_show_html_haml___3001049241409430540_70268641750820'
config/environments/production.rb:100:in `call'
app/controllers/editor/posts_controller.rb:66:in `preview'
app/middlewares/redirector.rb:25:in `response'
config/application.rb:44:in `call'
app/middlewares/redirector.rb:7:in `call'

This change needs daimon_markdown 0.7.0 or later

okkez-bot · 2017-02-28T03:34:24Z

Click here if you want to create rubocop's auto-correct Pull Request

See the documentation if you want to get more information about auto-correct.
Document

okkez-bot · 2017-02-28T03:34:24Z

test/lib/daimon_markdown/toc_pages_test.rb

+
+    test "extra white spaces in page separator" do
+      # in this case page separator doesn't work
+      body =<<~BODY


[rubocop]

Style/SpaceAroundOperators: Surrounding space missing for operator =. :ref

mtsmfm temporarily deployed to daimon-news-stg-pr-691 January 12, 2017 05:16 Inactive

okkez added the in progress label Jan 12, 2017

okkez changed the title ~~Add toc_pages plugin~~ [WIP] Add toc_pages plugin Jan 12, 2017

okkez added 5 commits January 17, 2017 13:45

Add toc_pages plugin

98773af

This is a daimon_markdown plugin.

Show toc in first page only

fdcbf99

Set id to header nodes properly

e61f71e

Search header node immediately after comment node strictly

7214f55

Add test for toc_pages daimon_markdown plugin

1c086ed

okkez temporarily deployed to daimon-news-stg-pr-691 January 17, 2017 04:46 Inactive

okkez force-pushed the toc-pages branch from d89416e to 1c086ed Compare January 17, 2017 04:46

okkez temporarily deployed to daimon-news-stg-pr-691 January 17, 2017 04:46 Inactive

okkez added the review label Jan 17, 2017

okkez changed the title ~~[WIP] Add toc_pages plugin~~ Add toc_pages plugin Jan 17, 2017

okkez removed the in progress label Jan 17, 2017

mtsmfm reviewed Jan 17, 2017

View reviewed changes

Stop using request object in toc_pages

d5380fe

okkez temporarily deployed to daimon-news-stg-pr-691 January 26, 2017 07:40 Inactive

Set context when preview

27f7d66

okkez temporarily deployed to daimon-news-stg-pr-691 January 26, 2017 08:16 Inactive

mtsmfm suggested changes Jan 31, 2017

View reviewed changes

Rename variable

ee13d54

@context -> @markdown_context It's straightforward.

okkez temporarily deployed to daimon-news-stg-pr-691 January 31, 2017 08:21 Inactive

Rename context key name full_text to original_text

2de75fa

Because `full_text` and `fullpath` don't have same form. It's very confusing us.

okkez temporarily deployed to daimon-news-stg-pr-691 January 31, 2017 08:37 Inactive

mtsmfm approved these changes Feb 23, 2017

View reviewed changes

Merge branch 'master' into toc-pages

9105744

mtsmfm temporarily deployed to daimon-news-stg-pr-691 February 23, 2017 01:14 Inactive

okkez-bot reviewed Feb 23, 2017

View reviewed changes

mtsmfm reviewed Feb 23, 2017

View reviewed changes

okkez added 2 commits February 23, 2017 12:03

Use Array#map properly

2099633

Remove redundant assignment

3ff4b8d

mtsmfm reviewed Feb 23, 2017

View reviewed changes

Use #at instead of #search with first

b85dfdf

`#at` returns first element which is matched condition.

okkez temporarily deployed to daimon-news-stg-pr-691 February 23, 2017 03:11 Inactive

okkez added 2 commits February 23, 2017 18:51

Add Header, UnorderedList, ListItem to simplify code

e047767

Fix for rubocop suggestions

e1528fb

okkez-bot reviewed Feb 23, 2017

View reviewed changes

okkez temporarily deployed to daimon-news-stg-pr-691 February 23, 2017 09:54 Inactive

okkez temporarily deployed to daimon-news-stg-pr-691 February 23, 2017 09:56 Inactive

Merge branch 'master' into toc-pages

2e6ca61

okkez temporarily deployed to daimon-news-stg-pr-691 February 24, 2017 01:30 Inactive

Generate nested ul-li list properly

6d9442d

ul must not be child of ul directry.

okkez temporarily deployed to daimon-news-stg-pr-691 February 24, 2017 06:17 Inactive

Remove empty else clause

1c7b692

okkez-bot reviewed Feb 24, 2017

View reviewed changes

okkez temporarily deployed to daimon-news-stg-pr-691 February 24, 2017 06:21 Inactive

Merge branch 'master' into toc-pages

2b1ae95

okkez temporarily deployed to daimon-news-stg-pr-691 February 24, 2017 06:26 Inactive

mtsmfm temporarily deployed to daimon-news-stg-pr-691 February 24, 2017 06:32 Inactive

okkez added 2 commits February 28, 2017 12:31

Check number of headers in the page

5702061

This change needs daimon_markdown 0.7.0 or later

Add missing space around = operator

015f0d7

okkez temporarily deployed to daimon-news-stg-pr-691 February 28, 2017 03:33 Inactive

okkez-bot reviewed Feb 28, 2017

View reviewed changes

okkez deployed to daimon-news-stg-pr-691 February 28, 2017 03:35 View deployment


		request = context[:request]

		current_page = if request.params.key?(:page)

Comments

Conversation

okkez commented Jan 12, 2017

Uh oh!

mtsmfm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

okkez commented Jan 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtsmfm commented Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

okkez commented Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

okkez-bot commented Feb 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mtsmfm commented Feb 28, 2017 •

edited

Loading

okkez commented Feb 28, 2017 •

edited

Loading