From c9c02ef3c09cf3662ba98957455409f7397ec109 Mon Sep 17 00:00:00 2001 From: Andy Zhao Date: Mon, 21 May 2018 17:35:03 -0400 Subject: [PATCH] [Done] Fix div in `li` issue (#320) * Lint and clean up github issue styles * Use heredoc HTML for easier formatting * Add better error handling for 404 github issues * Use custom sanitizer for markdown render * Refactor unless to ternary * Rename file and tighten up sanitization * Fix codepen and instagram tag styling --- app/assets/stylesheets/ltags/GithubTag.scss | 23 ++++++ .../stylesheets/ltags/InstagramTag.scss | 1 - app/controllers/articles_controller.rb | 8 +-- app/helpers/application_helper.rb | 71 +++++++++---------- app/labor/markdown_parser.rb | 2 +- app/liquid_tags/codepen_tag.rb | 8 +-- app/liquid_tags/github_tag.rb | 66 ++++++++--------- app/models/github_issue.rb | 23 ++---- app/sanitizers/rendered_markdown_scrubber.rb | 23 ++++++ app/views/comments/index.html.erb | 2 +- config/application.rb | 1 + .../codepen_liquid_tag.approved.html | 2 +- .../github_liquid_tag_default.approved.html | 22 +++++- 13 files changed, 150 insertions(+), 102 deletions(-) create mode 100644 app/sanitizers/rendered_markdown_scrubber.rb diff --git a/app/assets/stylesheets/ltags/GithubTag.scss b/app/assets/stylesheets/ltags/GithubTag.scss index 6f8ff904e..1863e0e1a 100644 --- a/app/assets/stylesheets/ltags/GithubTag.scss +++ b/app/assets/stylesheets/ltags/GithubTag.scss @@ -122,6 +122,29 @@ .edit-comment-hide { padding: 15px 15px; + h1{ + margin-left: 0; + font-size: 2em; + padding-bottom: 0.3em; + border-bottom: 1px solid #eaecef; + font-weight: 600; + } + + h3{ + margin-top: 24px; + margin-bottom: 16px; + padding: 0; + font-size: 1.25em; + font-weight: 600; + line-height: 1.25; + } + + h4{ + margin-top: 24px; + margin-bottom: 16px; + padding: 0; + } + *:first-child { margin-top: 0 !important; } diff --git a/app/assets/stylesheets/ltags/InstagramTag.scss b/app/assets/stylesheets/ltags/InstagramTag.scss index 37beb9211..90581c3bc 100644 --- a/app/assets/stylesheets/ltags/InstagramTag.scss +++ b/app/assets/stylesheets/ltags/InstagramTag.scss @@ -4,7 +4,6 @@ margin: 1px 1px 12px; max-width: 450px; width: calc(100% - 2px); - border-radius: 4px; box-shadow: none; display: block; padding: 0px; diff --git a/app/controllers/articles_controller.rb b/app/controllers/articles_controller.rb index 994163e3d..b5851834f 100644 --- a/app/controllers/articles_controller.rb +++ b/app/controllers/articles_controller.rb @@ -86,10 +86,10 @@ class ArticlesController < ApplicationController @article.tag_list = [] @article.main_image = nil edited_at_date = if @article.user == current_user && @article.published - DateTime.now - else - @article.edited_at - end + Time.now + else + @article.edited_at + end if @article.update(article_params.merge(edited_at: edited_at_date)) handle_org_assignment handle_hiring_tag diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 4409465ec..03ab567a3 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -1,5 +1,4 @@ module ApplicationHelper - def user_logged_in_status user_signed_in? ? "logged-in" : "logged-out" end @@ -20,6 +19,7 @@ module ApplicationHelper controller_name == "registrations" || controller_name == "users" || controller_name == "pages" || + controller_name == "dashboards"|| controller_name == "moderations"|| controller_name == "videos"|| @@ -39,7 +39,7 @@ module ApplicationHelper page_title end - def icon(name,pixels="20") + def icon(name, pixels = "20") image_tag icon_url(name), alt: name, class:"icon-img", height: pixels, width: pixels end @@ -47,25 +47,25 @@ module ApplicationHelper prefix = "https://res.cloudinary.com/practicaldev/image/upload/" case name when "twitter" - url = prefix+"v1456342401/twitter-logo-silhouette_1_letrqc.png" + prefix + "v1456342401/twitter-logo-silhouette_1_letrqc.png" when "github" - url = prefix+"v1456342401/github-logo_m841aq.png" + prefix + "v1456342401/github-logo_m841aq.png" when "link" - url = prefix+"v1456342401/link-symbol_apfbll.png" + prefix + "v1456342401/link-symbol_apfbll.png" when "volume" - url = prefix+"v1461589297/technology_1_aefet2.png" + prefix + "v1461589297/technology_1_aefet2.png" when "volume-mute" - url = prefix+"v1461589297/technology_jiugwb.png" + prefix + "v1461589297/technology_jiugwb.png" else - url = prefix+"v1456342953/star-in-black-of-five-points-shape_sor40l.png" + prefix + "v1456342953/star-in-black-of-five-points-shape_sor40l.png" end end - def cloudinary(url,width=nil,quality=80,format="jpg") + def cloudinary(url, width = nil, quality = 80, format = "jpg") if Rails.env.development? && (url.blank? || !url.include?("http")) return url end - if url && url.size > 0 + if url && url.size.positive? if width "https://res.cloudinary.com/practicaldev/image/fetch/c_scale,fl_progressive,q_auto,w_#{width}/f_auto/#{url}" else @@ -78,19 +78,19 @@ module ApplicationHelper def cloud_cover_url(url) return nil unless url.present? - width = 1000 - height = 420 - quality = "auto" + width = 1000 + height = 420 + quality = "auto" - cl_image_path(url, - :type=>"fetch", - :width => width, - :height => height, - :crop => "imagga_scale", - :quality => quality, - :flags => "progressive", - :fetch_format => "auto", - :sign_url => true) + cl_image_path(url, + type: "fetch", + width: width, + height: height, + crop: "imagga_scale", + quality: quality, + flags: "progressive", + fetch_format: "auto", + sign_url: true) end def cloud_social_image(article) @@ -114,43 +114,38 @@ module ApplicationHelper def tag_colors(tag) Rails.cache.fetch("view-helper-#{tag}/tag_colors", expires_in: 5.hours) do if found_tag = Tag.find_by_name(tag) - {background:found_tag.bg_color_hex,color:found_tag.text_color_hex} + { background: found_tag.bg_color_hex,color: found_tag.text_color_hex } else - {background:"#d6d9e0",color:"#606570"} + { background: "#d6d9e0", color: "#606570" } end end end def beautified_url(url) - begin - url.sub(/^((http[s]?|ftp):\/)?\//, '').sub(/\?.*/, '').chomp('/') - rescue - url - end + url.sub(/^((http[s]?|ftp):\/)?\//, "").sub(/\?.*/, "").chomp("/") + rescue + url end def org_bg_or_white(org) - return "#ffffff" unless (org && org.bg_color_hex) - org.bg_color_hex + org&.bg_color_hex ? org.bg_color_hex : "#ffffff" end - def sanitized_article_body(processed_html) + def sanitize_rendered_markdown(processed_html) ActionController::Base.helpers.sanitize processed_html.html_safe, - tags: %w(button strong em a table tbody thead tfoot th tr td col colgroup del p h1 h2 h3 h4 h5 h6 blockquote time div span i em u b ul ol li dd dl dt q code pre img sup sub cite center br small hr video source figcaption add ruby rp rt), - attributes: %w(href strong em class ref rel src title alt colspan height width size rowspan span value start data-conversation data-lang data-no-instant data-url id name type loop) + scrubber: RenderedMarkdownScrubber.new end def sanitized_sidebar(text) ActionController::Base.helpers.sanitize simple_format(text), - tags: %w(p, b, i, em, strike, strong, u, br), - attributes: %w() + tags: %w(p b i em strike strong u br) end - def track_split_version(url,version) + def track_split_version(url, version) "trackOutboundLink('#{url}','#{version}'); return false;" end - def follow_button(followable,style="full") + def follow_button(followable, style = "full") "".html_safe end diff --git a/app/labor/markdown_parser.rb b/app/labor/markdown_parser.rb index d04e655b1..1b662727f 100644 --- a/app/labor/markdown_parser.rb +++ b/app/labor/markdown_parser.rb @@ -64,7 +64,7 @@ class MarkdownParser catch_xss_attempts(@content) escaped_content = escape_liquid_tags_in_codeblock(@content) html = markdown.render(escaped_content) - sanitized_content = sanitized_article_body(html) + sanitized_content = sanitize_rendered_markdown(html) begin parsed_liquid = Liquid::Template.parse(sanitized_content) rescue StandardError => e diff --git a/app/liquid_tags/codepen_tag.rb b/app/liquid_tags/codepen_tag.rb index 8e1301436..6aef14bb4 100644 --- a/app/liquid_tags/codepen_tag.rb +++ b/app/liquid_tags/codepen_tag.rb @@ -2,17 +2,17 @@ class CodepenTag < LiquidTagBase def initialize(tag_name, link, tokens) super @link = parse_link(link) - @height = 500 + @height = 600 end def render(context) - html = <<~HTML + html = <<-HTML HTML finalize_html(html) diff --git a/app/liquid_tags/github_tag.rb b/app/liquid_tags/github_tag.rb index e3625441c..dce01a117 100644 --- a/app/liquid_tags/github_tag.rb +++ b/app/liquid_tags/github_tag.rb @@ -15,36 +15,36 @@ class GithubTag < LiquidTagBase end def render(context) - contentJSON = @content.issue_serialized + content_json = @content.issue_serialized body = @content.processed_html - username = contentJSON[:user][:login] - user_html_url = contentJSON[:user][:html_url] - user_avatar_url = contentJSON[:user][:avatar_url] - date = Date.parse(contentJSON[:created_at].to_s).strftime('%b %d, %Y') - date_link = contentJSON[:html_url] + username = content_json[:user][:login] + user_html_url = content_json[:user][:html_url] + user_avatar_url = content_json[:user][:avatar_url] + date = Date.parse(content_json[:created_at].to_s).strftime("%b %d, %Y") + date_link = content_json[:html_url] title = generate_title - html = '' \ - "
"\ - "#{title}" \ - "
" \ - "
" \ - " " \ - "\"#{username} " \ - " " \ - " " \ - " " \ - "
" \ - " " \ - "#{username} " \ - " commented on #{date} " \ - "
" \ - "
" \ - "
" \ - "#{body.chomp} " \ - "
" \ - "
" \ - "
" - + html = <<-HTML +
+ #{title} +
+
+ + #{username} avatar + + + +
+ + #{username} + commented on #{date} +
+
+
+ #{body.chomp} +
+
+
+ HTML finalize_html(html) end @@ -52,7 +52,7 @@ class GithubTag < LiquidTagBase def parse_link(link) link = ActionController::Base.helpers.strip_tags(link) - link_no_space = link.delete(' ') + link_no_space = link.delete(" ") if valid_link?(link_no_space) generate_api_link(link_no_space) else @@ -69,10 +69,10 @@ class GithubTag < LiquidTagBase end def generate_title - contentJSON = @content.issue_serialized - title = contentJSON[:title] - number = contentJSON[:number] - link = contentJSON[:html_url] + content_json = @content.issue_serialized + title = content_json[:title] + number = content_json[:number] + link = content_json[:html_url] return unless title "

" \ "" \ diff --git a/app/models/github_issue.rb b/app/models/github_issue.rb index 1113c178e..edd3c2dc3 100644 --- a/app/models/github_issue.rb +++ b/app/models/github_issue.rb @@ -6,24 +6,13 @@ class GithubIssue < ApplicationRecord find_by_url(url) || fetch(url) end - # def self.fetch(url) - # begin - # return try_to_get_issue(url) - # rescue StandardError => e - # raise StandardError, e.message - # end - # end - def self.fetch(url) - tries = 0 - issue = nil - until tries > 4 || issue - begin - return issue = try_to_get_issue(url) - rescue => e - puts e - tries += 1 - end + return try_to_get_issue(url) + rescue StandardError => e + if e.message.include?("404 - Not Found") + raise StandardError, "A GitHub issue 404'ed and could not be found!" + else + raise StandardError, e.message end end diff --git a/app/sanitizers/rendered_markdown_scrubber.rb b/app/sanitizers/rendered_markdown_scrubber.rb new file mode 100644 index 000000000..ce73e982c --- /dev/null +++ b/app/sanitizers/rendered_markdown_scrubber.rb @@ -0,0 +1,23 @@ +class RenderedMarkdownScrubber < Rails::Html::PermitScrubber + def initialize + super + self.tags = %w(a add b blockquote br button center cite code col colgroup dd del dl dt em em figcaption h1 h2 h3 h4 h5 h6 hr i img li ol p pre q rp rt ruby small source span strong sub sup table tbody td tfoot th thead time tr u ul video) + self.attributes = %w(alt class colspan data-conversation data-lang data-no-instant data-url em height href id loop name ref rel rowspan size span src start strong title type value width) + end + + def allowed_node?(node) + @tags.include?(node.name) || valid_codeblock_div?(node) + end + + private + + def valid_codeblock_div?(node) + node.name == "div" && + node.attributes.count == 1 && + node.children.first.name == "pre" && + node.parent.name == "#document-fragment" && + node.attributes.values.any? do |attribute_value| + attribute_value.value == "highlight" + end + end +end diff --git a/app/views/comments/index.html.erb b/app/views/comments/index.html.erb index 1ad4a0b4f..8e159cf1e 100644 --- a/app/views/comments/index.html.erb +++ b/app/views/comments/index.html.erb @@ -52,7 +52,7 @@ <% if @commentable.processed_html.present? %> <% if @commentable.processed_html.size < 350 %>
- <%= sanitized_article_body(@commentable.processed_html.html_safe) %> + <%= sanitize_rendered_markdown(@commentable.processed_html.html_safe) %>
<% else %>
diff --git a/config/application.rb b/config/application.rb index b6b0a7386..5c774f415 100644 --- a/config/application.rb +++ b/config/application.rb @@ -32,6 +32,7 @@ module PracticalDeveloper config.autoload_paths += Dir["#{config.root}/app/liquid_tags/"] config.autoload_paths += Dir["#{config.root}/app/observers/"] config.autoload_paths += Dir["#{config.root}/app/black_box/"] + config.autoload_paths += Dir["#{config.root}/app/sanitizers"] config.autoload_paths += Dir["#{config.root}/lib/"] config.active_record.observers = :article_observer, :reaction_observer diff --git a/spec/support/fixtures/approvals/codepen_liquid_tag.approved.html b/spec/support/fixtures/approvals/codepen_liquid_tag.approved.html index d4fc95ffd..49e7330f3 100644 --- a/spec/support/fixtures/approvals/codepen_liquid_tag.approved.html +++ b/spec/support/fixtures/approvals/codepen_liquid_tag.approved.html @@ -1,6 +1,6 @@ - + diff --git a/spec/support/fixtures/approvals/github_liquid_tag_default.approved.html b/spec/support/fixtures/approvals/github_liquid_tag_default.approved.html index 16b033ba9..da785af30 100644 --- a/spec/support/fixtures/approvals/github_liquid_tag_default.approved.html +++ b/spec/support/fixtures/approvals/github_liquid_tag_default.approved.html @@ -1,7 +1,22 @@ -

[Feature] granular forceUpdate #9218

thysultan avatar
thysultan commented on Mar 19, 2017

When forceUpdate is called on a long list at the very least the whole list must be traversed. For example with the following list arr = [1, 2, 3, 4, 5, 6] and if we where to push 7 arr.push(7) the reconciler will go through elements 1 - 6 and then add a 7 at the end of that list. The suggestion to add support for the following forceUpdate signature.

+
+

[Feature] granular forceUpdate #9218

+
+
+ + thysultan avatar + + +
+ + thysultan + commented on Mar 19, 2017 +
+
+
+

When forceUpdate is called on a long list at the very least the whole list must be traversed. For example with the following list arr = [1, 2, 3, 4, 5, 6] and if we where to push 7 arr.push(7) the reconciler will go through elements 1 - 6 and then add a 7 at the end of that list. The suggestion to add support for the following forceUpdate signature.

forceUpdate(a: {function|number}, b: number?)

Where when passed as an integer a is the start index and b the number of items to traverse similar to how str.substr() works.

When the function encounters a number as the a argument it changes the start index of where to start when reconciling the components children and when the b argument is set it changes the end index of where to end the traversal of the components children. So with the above mentioned example if i wanted to optimize the reconciliation process for append updates i could do the following.

@@ -13,6 +28,9 @@ 
setState(a: {function|Object}, b: (function|number)?, c: {number})
 
 // usage
-setState({arr: [...]}, this.state.arr.length);
+setState({arr: [...]}, this.state.arr.length);
+
+
+