-
Notifications
You must be signed in to change notification settings - Fork 17
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
base: master
Are you sure you want to change the base?
fixes #7
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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>/) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
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' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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']}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"\ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
"</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>" | ||
|
@@ -31,4 +31,4 @@ | |
expect(posting.article_with_image).to eq(response) | ||
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.