From 5ba5a9f3d605b206097021b1ee89380c4f2011e4 Mon Sep 17 00:00:00 2001 From: Sam Date: Thu, 5 Mar 2015 15:01:17 +1100 Subject: [PATCH] UX: fix jerky UI when creating new posts - do not scroll screen if post is already on screen - do not hide/show suggested topics each time you post - be less aggressive about setting scrollTop in LockOn --- .../discourse/controllers/topic.js.es6 | 14 +++---- app/assets/javascripts/discourse/lib/url.js | 39 ++++++++++++++----- .../discourse/models/post-stream.js.es6 | 10 ++++- vendor/assets/javascripts/lock-on.js | 38 +++++++++++++++--- 4 files changed, 76 insertions(+), 25 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/topic.js.es6 b/app/assets/javascripts/discourse/controllers/topic.js.es6 index 5f855847149..b23e7459c74 100644 --- a/app/assets/javascripts/discourse/controllers/topic.js.es6 +++ b/app/assets/javascripts/discourse/controllers/topic.js.es6 @@ -44,16 +44,12 @@ export default ObjectController.extend(Discourse.SelectedPostsCount, BufferedCon }.observes('controllers.search.term', 'controllers.header.visibleDropdown'), postStreamLoadedAllPostsChanged: function(){ - // hold back rendering 1 run loop for every transition. - var self = this; + // in the past we would hold back rendering of suggested topics + // 1 run loop, however post optimisations rendering them is fast + // holding back rendering just makes for a more jerky UI on initial + // transition var loaded = this.get('postStream.loadedAllPosts'); - this.set('loadedAllPosts', false); - - if(loaded){ - Em.run.next(function(){ - self.set('loadedAllPosts',true); - }); - } + this.set('loadedAllPosts', loaded); }.observes('postStream', 'postStream.loadedAllPosts'), diff --git a/app/assets/javascripts/discourse/lib/url.js b/app/assets/javascripts/discourse/lib/url.js index 00117831b28..fcbc2a5d526 100644 --- a/app/assets/javascripts/discourse/lib/url.js +++ b/app/assets/javascripts/discourse/lib/url.js @@ -14,23 +14,44 @@ Discourse.URL = Ember.Object.createWithMixins({ /** Jumps to a particular post in the stream **/ - jumpToPost: function(postNumber) { + jumpToPost: function(postNumber, opts) { var holderId = '#post-cloak-' + postNumber; + var offset = function(){ + + var $header = $('header'), + $title = $('#topic-title'), + windowHeight = $(window).height() - $title.height(), + expectedOffset = $title.height() - $header.find('.contents').height() + (windowHeight / 5); + + return $header.outerHeight(true) + ((expectedOffset < 0) ? 0 : expectedOffset); + }; + + Em.run.schedule('afterRender', function() { if (postNumber === 1) { $(window).scrollTop(0); return; } - new LockOn(holderId, {offsetCalculator: function() { - var $header = $('header'), - $title = $('#topic-title'), - windowHeight = $(window).height() - $title.height(), - expectedOffset = $title.height() - $header.find('.contents').height() + (windowHeight / 5); + var lockon = new LockOn(holderId, {offsetCalculator: offset}); + var holder = $(holderId); + + if(holder.length > 0 && opts && opts.skipIfOnScreen){ + // if we are on screen just scroll to post + var elementTop = lockon.elementTop(), + scrollTop = $(window).scrollTop(), + windowHeight = $(window).height()-offset(), + height = holder.height(); + + if (elementTop > scrollTop && + (elementTop + height) < (scrollTop + windowHeight)) { + return; + } + } + + lockon.lock(); - return $header.outerHeight(true) + ((expectedOffset < 0) ? 0 : expectedOffset); - }}).lock(); }); }, @@ -218,7 +239,7 @@ Discourse.URL = Ember.Object.createWithMixins({ progressController.set('progressPosition', progress); self.appEvents.trigger('post:highlight', closest); }).then(function() { - Discourse.URL.jumpToPost(closest); + Discourse.URL.jumpToPost(closest, {skipIfOnScreen: true}); }); // Abort routing, we have replaced our state. diff --git a/app/assets/javascripts/discourse/models/post-stream.js.es6 b/app/assets/javascripts/discourse/models/post-stream.js.es6 index 8116e41be73..7561ad21855 100644 --- a/app/assets/javascripts/discourse/models/post-stream.js.es6 +++ b/app/assets/javascripts/discourse/models/post-stream.js.es6 @@ -35,7 +35,15 @@ const PostStream = Ember.Object.extend({ }.property('stream.@each'), loadedAllPosts: function() { - if (!this.get('hasLoadedData')) { return false; } + if (!this.get('hasLoadedData')) { + return false; + } + + // if we are staging a post assume all is loaded + if (this.get('lastPostId') === -1) { + return true; + } + return !!this.get('posts').findProperty('id', this.get('lastPostId')); }.property('hasLoadedData', 'posts.@each.id', 'lastPostId'), diff --git a/vendor/assets/javascripts/lock-on.js b/vendor/assets/javascripts/lock-on.js index e58a2928327..c7c1ec1cb49 100644 --- a/vendor/assets/javascripts/lock-on.js +++ b/vendor/assets/javascripts/lock-on.js @@ -1,3 +1,21 @@ +// Dear traveller, you are entering a zone where we are at war with the browser +// the browser is insisting on positioning scrollTop per the location it was in +// the past, we are insisting on it being where we want it to be +// The hack is just to keep trying over and over to positiong the scrollbar (up to 1 minute) +// +// The root cause is that a "refresh" on a topic page will almost never be at the +// same position it was in the past, the URL points to the post at the top of the +// page, so a refresh will try to bring that post into view causing drift +// +// Additionally if you loaded multiple batches of posts, on refresh they will not +// be loaded. +// +// This hack leads to a slight jerky experience, however other workarounds are more +// complex, the 2 options we have are +// +// 1. onbeforeunload ensure we are scrolled to the right spot +// 2. give up on the scrollbar and implement it ourselves (something that will happen) + (function (exports) { var scrollEvents = "scroll.lock-on touchmove.lock-on mousedown.lock-on wheel.lock-on DOMMouseScroll.lock-on mousewheel.lock-on keyup.lock-on"; @@ -19,11 +37,15 @@ LockOn.prototype.lock = function() { var self = this, previousTop = this.elementTop(), - startedAt = new Date().getTime() + startedAt = new Date().getTime(), i = 0; $(window).scrollTop(previousTop); + var within = function(threshold,x,y) { + return Math.abs(x-y) < threshold; + }; + var interval = setInterval(function() { i = i + 1; @@ -31,20 +53,24 @@ scrollTop = $(window).scrollTop(); if (typeof(top) === "undefined") { - $('body,html').off(scrollEvents) + $('body,html').off(scrollEvents); clearInterval(interval); return; } - if ((top !== previousTop) || (scrollTop !== top)) { + if (!within(4, top, previousTop) || !within(4, scrollTop, top)) { $(window).scrollTop(top); + // animating = true; + // $('html,body').animate({scrollTop: parseInt(top,10)+'px'}, 200, 'swing', function(){ + // animating = false; + // }); previousTop = top; } - // We commit suicide after 1s just to clean up + // We commit suicide after 3s just to clean up var nowTime = new Date().getTime(); if (nowTime - startedAt > 1000) { - $('body,html').off(scrollEvents) + $('body,html').off(scrollEvents); clearInterval(interval); } @@ -55,7 +81,7 @@ $('body,html').off(scrollEvents); clearInterval(interval); } - }) + }); };