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.

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

fixes #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
22 changes: 21 additions & 1 deletion app/models/article.rb
Original file line number Diff line number Diff line change
@@ -1,2 +1,22 @@
class Article < Posting
end
# method should return hash or nil (not string)
def article_with_image
figure_matches = body.to_s.match(/<figure.*?>.*?(<img.*?>.*?<\/img>|<img.*?\/>).*?(.*?(<img.*?>.*?<\/img>|<img.*?\/>).*?)*<\/figure>/)
Copy link
Author

Choose a reason for hiding this comment

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

I've used regex since it allows to extract the first image only and takes less code for string parsing

Copy link
Author

Choose a reason for hiding this comment

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

previous implementation returned string in case of issues - it was not handled on views and it is not the best practice to return hash and string types - or raising errors or nil.

Copy link
Author

Choose a reason for hiding this comment

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

I've converted body.to_s to fix issue with nil body values

return unless figure_matches
first_image = figure_matches[1]
posting_image_params(first_image)
end

private

def posting_image_params(html)
Copy link
Author

Choose a reason for hiding this comment

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

  1. the issue in previous implementation was that if we pass here html with multiple images and some of attributes are missing for first image - then the response is inconsistent: some attr are for first image and missing ones are from next image
  2. also I've included bot '' and "" options for html values which was missing in previous implementation

Copy link
Author

Choose a reason for hiding this comment

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

i've removed lambda since it only adds complexity to the code and is not needed

tag_attributes = {}

%w[alt src data-image].each do |attribute|
data = html.match(/#{attribute}=("(.+?)"|'(.+?)')/)
tag_attributes[attribute] = data[2] || data[3] if data
end
# tag_parse
tag_attributes
end
end
32 changes: 1 addition & 31 deletions app/models/posting.rb
Original file line number Diff line number Diff line change
@@ -1,34 +1,4 @@
class Posting < ApplicationRecord

belongs_to :author, class_name: 'User', foreign_key: 'user_id'
belongs_to :editor, class_name: 'User', foreign_key: 'editor_id'

def article_with_image
return type if type != 'Article'
Copy link
Author

Choose a reason for hiding this comment

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

I've removed this checking and moved out article methods to appropriate class


figure_start = body.index('<figure')
figure_end = body.index('</figure>')
return "#{figure_start}_#{figure_end}" if figure_start.nil? || figure_end.nil?

image_tags = body[figure_start...figure_end + 9]
return 'not include <img' unless image_tags.include?('<img')

posting_image_params(image_tags)
end

private

def posting_image_params(html)
tag_parse = -> (image, att) { image.match(/#{att}="(.+?)"/) }
tag_attributes = {}

%w[alt src data-image].each do |attribute|
data = tag_parse.(html, attribute)
unless data.nil?
tag_attributes[attribute] = data[1] unless data.size < 2
end
end
# tag_parse
tag_attributes
end
end
end
4 changes: 2 additions & 2 deletions app/views/articles/_render_postng_snippet.html.slim
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
-image = posting.article_with_image
-image = posting.respond_to?(:article_with_image) ? posting.article_with_image : nil
Copy link
Author

Choose a reason for hiding this comment

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

it fixes the issue with strings we had before: now we have nil or hash

-if image
figure id="img_#{posting.id}"
img src="#{image['src']}" alt="#{image['alt'] || posting.title}" data-image="#{image['data-image']}"
Copy link
Author

Choose a reason for hiding this comment

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

we can also use symbol keys instead of string keys in the #article_with_image method response - it is better for ruby code performance

.teaser id="teaser_#{posting.id}"
=> posting_snippet(posting) # uses HTML_Truncator gem and calls .html_safe on the output
=> posting_snippet(posting) # uses HTML_Truncator gem and calls .html_safe on the output
Copy link
Author

Choose a reason for hiding this comment

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

posting_snippet helper is not implemented so it will not work, but we can implement it this way:

def posting_snippet(posting)
  posting.body.to_s.html_safe
end

4 changes: 2 additions & 2 deletions specs/posting_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"your feedback below<strong>!</strong></p>\r\n<figure><img src=\"https://images."\
"peerspot.com/image/upload/c_limit,f_auto,q_auto,w_550/bvvrzbv97pp5srg612"\
"le16pv99rg.jpg\" data-image=\"27c79574-7aa7-4eea-8515-d2a128698803.jpg\" alt=\"Spotlight"\
" #3 - a community digest\"></figure>\r\n<p><strong><br></strong></p>\r\n<h2>Trending"\
" #3 - a community digest\"/></figure>\r\n<p><strong><br></strong></p>\r\n<h2>Trending"\
Copy link
Author

Choose a reason for hiding this comment

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

I've added correct ending of the tag '/>' (before it was just '>')

Copy link
Author

Choose a reason for hiding this comment

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

btw i'm handling both tag style cases in article_with_image implementation:

"</h2>\r\n<ul>\r\n<li><a target=\"_blank\" href=\"https://www.peerspot.com/quest"\
"ions/looking-for-an-identity-and-access-management-product-for-an-energy-and-utility-organ"\
"ization\">Looking for an Identity and Access Management product for an energy and utility organization</a></li>"
Expand All @@ -31,4 +31,4 @@
expect(posting.article_with_image).to eq(response)
end
end
end
end