From d79aa917f123389d4405c213339f5adb8064c7b9 Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 8 Jul 2013 11:13:45 +1000 Subject: [PATCH] add option suppress_reply_directly_above to stop suppressing the reply directly above added a bunch of debugging information to help diagnose weird positioning issues --- .../javascripts/discourse/models/post.js | 5 +- .../javascripts/discourse/views/post_view.js | 16 +++- .../javascripts/discourse/views/topic_view.js | 76 ++++++++++++++++--- app/models/site_setting.rb | 1 + config/locales/server.en.yml | 2 + 5 files changed, 83 insertions(+), 17 deletions(-) diff --git a/app/assets/javascripts/discourse/models/post.js b/app/assets/javascripts/discourse/models/post.js index d048f1eb4..6c0701fa3 100644 --- a/app/assets/javascripts/discourse/models/post.js +++ b/app/assets/javascripts/discourse/models/post.js @@ -30,7 +30,10 @@ Discourse.Post = Discourse.Model.extend({ }.property('username'), showUserReplyTab: function() { - return this.get('reply_to_user') && (this.get('reply_to_post_number') < (this.get('post_number') - 1)); + return this.get('reply_to_user') && ( + !Discourse.SiteSettings.suppress_reply_directly_above || + this.get('reply_to_post_number') < (this.get('post_number') - 1) + ); }.property('reply_to_user', 'reply_to_post_number', 'post_number'), byTopicCreator: function() { diff --git a/app/assets/javascripts/discourse/views/post_view.js b/app/assets/javascripts/discourse/views/post_view.js index 56e88d761..1ba11e272 100644 --- a/app/assets/javascripts/discourse/views/post_view.js +++ b/app/assets/javascripts/discourse/views/post_view.js @@ -69,7 +69,16 @@ Discourse.PostView = Discourse.View.extend({ // Toggle visibility of parent post toggleParent: function(e) { var postView = this; + var post = this.get('post'); var $parent = this.$('.parent-post'); + var inReplyTo = post.get('reply_to_post_number'); + + if (post.get('post_number') - 1 === inReplyTo) { + // true means ... avoid scroll if possible + Discourse.TopicView.jumpToPost(post.get('topic_id'), inReplyTo, true); + return; + } + if (this.get('parentPost')) { $('nav', $parent).removeClass('toggled'); // Don't animate on touch @@ -80,11 +89,10 @@ Discourse.PostView = Discourse.View.extend({ $parent.slideUp(function() { postView.set('parentPost', null); }); } } else { - var post = this.get('post'); this.set('loadingParent', true); $('nav', $parent).addClass('toggled'); - Discourse.Post.loadByPostNumber(post.get('topic_id'), post.get('reply_to_post_number')).then(function(result) { + Discourse.Post.loadByPostNumber(post.get('topic_id'), inReplyTo).then(function(result) { postView.set('loadingParent', false); // Give the post a reference back to the topic result.topic = postView.get('post.topic'); @@ -159,9 +167,9 @@ Discourse.PostView = Discourse.View.extend({ showLinkCounts: function() { var postView = this; - var link_counts; + var link_counts = this.get('post.link_counts'); - if (link_counts = this.get('post.link_counts')) { + if (link_counts) { _.each(link_counts, function(lc) { if (lc.clicks > 0) { postView.$(".cooked a[href]").each(function() { diff --git a/app/assets/javascripts/discourse/views/topic_view.js b/app/assets/javascripts/discourse/views/topic_view.js index 973ce43c4..ee03a55fa 100644 --- a/app/assets/javascripts/discourse/views/topic_view.js +++ b/app/assets/javascripts/discourse/views/topic_view.js @@ -329,30 +329,82 @@ Discourse.TopicView = Discourse.View.extend(Discourse.Scrolling, { Discourse.TopicView.reopenClass({ // Scroll to a given post, if in the DOM. Returns whether it was in the DOM or not. - jumpToPost: function(topicId, postNumber) { + jumpToPost: function(topicId, postNumber, avoidScrollIfPossible) { Em.run.scheduleOnce('afterRender', function() { + var rows = $('.topic-post.ready'); + // Make sure we're looking at the topic we want to scroll to if (topicId !== parseInt($('#topic').data('topic-id'), 10)) { return false; } var $post = $("#post_" + postNumber); if ($post.length) { - if (postNumber === 1) { - $('html, body').scrollTop(0); + + var postTop = $post.offset().top; + var highlight = true; + + var header = $('header'); + var title = $('#topic-title'); + var expectedOffset = title.height() - header.find('.contents').height(); + console.log(expectedOffset); + + if (expectedOffset < 0) { + expectedOffset = 0; + } + + var offset = (header.outerHeight(true) + expectedOffset); + var windowScrollTop = $('html, body').scrollTop(); + + if (avoidScrollIfPossible && postTop > windowScrollTop + offset && postTop < windowScrollTop + $(window).height() + 100) { + // in view } else { - var header = $('header'); - var title = $('#topic-title'); - var expectedOffset = title.height() - header.find('.contents').height(); + // not in view ... bring into view + if (postNumber === 1) { + $(window).scrollTop(0); + highlight = false; + } else { + var desired = $post.offset().top - offset; + $(window).scrollTop(desired); - if (expectedOffset < 0) { - expectedOffset = 0; + // TODO @Robin, I am seeing multiple events in chrome issued after + // jumpToPost if I refresh a page, sometimes I see 2, sometimes 3 + // + // 1. Where are they coming from? + // 2. On refresh we should only issue a single scrollTop + // 3. If you are scrolled down in BoingBoing desired sometimes is wrong + // due to vanishing header, we should not be rendering it imho until after + // we render the posts + + var first = true; + var t = new Date(); + console.log("DESIRED:" + desired); + var enforceDesired = function(){ + if($(window).scrollTop() !== desired) { + console.log("GOT EVENT " + $(window).scrollTop()); + console.log("Time " + (new Date() - t)); + console.trace(); + if(first) { + $(window).scrollTop(desired); + first = false; + } + // $(document).unbind("scroll", enforceDesired); + } + }; + + // uncomment this line to help debug this issue. + // $(document).scroll(enforceDesired); } + } - $('html, body').scrollTop($post.offset().top - (header.outerHeight(true) + expectedOffset)); - + if(highlight) { var $contents = $('.topic-body .contents', $post); - var originalCol = $contents.css('backgroundColor'); - $contents.css({ backgroundColor: "#ffffcc" }).animate({ backgroundColor: originalCol }, 2500); + var origColor = $contents.data('orig-color') || $contents.css('backgroundColor'); + + $contents.data("orig-color", origColor); + $contents + .css({ backgroundColor: "#ffffcc" }) + .stop() + .animate({ backgroundColor: origColor }, 2500); } } }); diff --git a/app/models/site_setting.rb b/app/models/site_setting.rb index 1164ce6bc..3d157a1a8 100644 --- a/app/models/site_setting.rb +++ b/app/models/site_setting.rb @@ -41,6 +41,7 @@ class SiteSetting < ActiveRecord::Base client_setting(:min_search_term_length, 3) client_setting(:flush_timings_secs, 5) client_setting(:suppress_reply_directly_below, true) + client_setting(:suppress_reply_directly_above, true) client_setting(:email_domains_blacklist, 'mailinator.com') client_setting(:email_domains_whitelist) client_setting(:version_checks, true) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 2745d5c93..516842b49 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -514,6 +514,8 @@ en: system_username: "Username for the author of automated private messages sent by the forum" send_welcome_message: "Do new users get a welcome private message?" suppress_reply_directly_below: "Don't show reply count on a post when there is a single reply directly below" + suppress_reply_directly_above: "Don't show in-reply-to on a post when there is a single reply directly above" + allow_index_in_robots_txt: "Site should be indexed by search engines (update robots.txt)" email_domains_blacklist: "A pipe-delimited list of email domains that are not allowed. Example: mailinator.com|trashmail.net" email_domains_whitelist: "A pipe-delimited list of email domains that users may register with. WARNING: Users with email domains other than those listed will not be allowed."