6
\$\begingroup\$

I'm developing a publishing engine and trying to find better ways to optimize the code. The end result is to have an open-source alternative to publish content on mobile devices.

For now, though, I have a touch-scroll-slide system I have created from a modified scroll system found elsewhere. There is a peculiar hesitation while doing slides (horizontally) on a mobile device (mostly iPad 1) while in portrait, yet the landscape version works very well. It is jQuery-based.

I realize there are other portions of the larger project which may be dragging the behaviour down, but don't want to overwhelm people with code at this time. The end result (so far) is viewable here.

The code function for scroll-slide is here:

 (function($){
 $.fn.jScrollTouch = function () {
 return this.each(function() {
 // The two concepts for each page
 // scroller = text scrolling area
 // slider = full-screen page
 //previous = the page before the current
 // next = the page after the current
 var scroller = $(this).find('.article-scroller');
 var slider = $(this);
 var previous = slider.prev();
 var next = slider.next();
 var slideOn = true;
 if (slider.attr('id')=='overlayed'){ slideOn = false;
 //console.log(slider.attr('id'))
 };
 scroller.css({'overflow': 'auto','position':'relative'}); 
 /*
 if (mobile) {
 //console.log('touch only');
 scroller.css({'overflow': 'hidden','position':'relative'});
 } else {
 //console.log('desk only');
 scroller.css({'overflow': 'auto','position':'relative'});
 }*/
 var height = 0;
 var cpos = scroller.scrollTop()
 scroller.scrollTop(100000);
 height = scroller.scrollTop();
 scroller.scrollTop(cpos);
 var fullheight = height + scroller.outerHeight();
 var scrollbarV_length = scroller.innerHeight()*(scroller.innerHeight()/fullheight)+2;
 var width = 0;
 minWidth=($("body").width())*0.15;
 var lpos = scroller.scrollLeft();
 scroller.scrollLeft(100000);
 width = scroller.scrollLeft();
 scroller.scrollLeft(lpos);
 var fullwidth = width + scroller.outerWidth();
 var scrollbarH_length = scroller.innerWidth()*(scroller.innerWidth()/fullwidth)+2;
 if(mobile){
 var scrollbarV = $('<div></div>');
 scrollbarV.css({ 'display':'none',
 'position':'absolute',
 'width':'5px',
 'height':scrollbarV_length+'px',
 'left':width-10+'px',
 'top':0,'background':'black',
 'border':'1px white solid',
 '-webkit-border-radius':'5px',
 'opacity':'0.9' });
 var scrollbarH = $('<div></div>'); //not used
 scrollbarH.css({ 'display':'none',
 'position':'absolute',
 'height':'5px',
 'width':scrollbarH_length+'px',
 'top':scroller.innerHeight()-7+'px',
 'left':0,'background':'black',
 'border':'1px white solid',
 '-webkit-border-radius':'5px',
 'opacity':'0.9'});
 if(height) scroller.append(scrollbarV);
 if(width) scroller.append(scrollbarH);
 }
 // these two chceks are to prevent sliding if
 // slider is animated or it is overlayed image
 currentPage = JSON.parse(localStorage.getItem('currentPage'));
 $remover = $('.article-wrapper:eq(' + currentPage + ')');
 $remover.stop();
 if (!$remover.is(':animated')) {
 slider.bind('mousedown touchstart',function(e){
 width=$("body").width();
 height=$("body").height();
 if(mobile){
 e = e.originalEvent.touches[0];
 scrollbarV.show();
 scrollbarH.show();
 }
 if (slideOn==true){ 
 $('.active').toggleClass('active');
 slider.stop(false, true).addClass('active').removeClass('hidder');
 }
 $('.article-wrapper').not('.active').addClass('hidder');
 if(previous.is(':animated')){
 previous.stop(true, true).removeClass('hidder');
 }
 if (next.is(':animated')){
 next.stop(true, true).removeClass('hidder');
 }
 // previous.css('style','');
 // next.css('style','');
 var initX = e.pageX;
 var sX = e.pageX;
 var sWX = 0;
 var initY = e.pageY;
 var sY = e.pageY;
 var sWY = 0;
 var prevsWX = 0; //stores the previous sWX
 var scrollDirection = 0;
 var nextpage = 0;
 var display = false;
 var displayed = false;
 cpos = scroller.scrollTop();
 slider.bind('mousemove touchmove ',function(ev){
 if(mobile){
 ev.preventDefault();
 ev = ev.originalEvent.touches[0];
 } 
 // console.log('started');
 // currentPage=parseInt(slider.attr("id"));
 var top = cpos-(ev.pageY-sY);
 var left = lpos-(ev.pageX-sX);
 sWX = sWX-(sX-ev.pageX);
 sX = ev.pageX;
 sWY = sWY-(sY-ev.pageY);
 sY = ev.pageY;
 var horDistance = Math.abs(sWX);
 var verDistance = Math.abs(sWY);
 if (scrollDirection ==0){ // haven't checked direction yet
 if ( verDistance < horDistance) {
 scrollDirection = 1; // moving horizontally
 } else if ( verDistance > horDistance) { 
 scrollDirection = 2; // moving vertically
 }
 }
 if (scrollDirection == 2 ){ //set up the scrolling movement
 //set up the scroll bars
 scroller.scrollTop(top);
 cpos = scroller.scrollTop(); 
 sY = ev.pageY;
 if(mobile){
 scrollbarV.css({ 'left':Math.min(scroller.innerWidth()-7+lpos,fullwidth)+'px',
 'top':Math.min(cpos+cpos*scroller.innerHeight()/fullheight,fullheight-scrollbarV_length)+'px'});
 scrollbarH.css({ 'top':Math.min(scroller.innerHeight()-7+cpos,fullheight)+'px',
 'left':Math.min(lpos+lpos*scroller.innerWidth()/fullwidth,fullwidth-scrollbarH_length)+'px'});
 }
 }
 if (scrollDirection == 1 && slideOn==true){ //set up the sideways movement
 // console.log('continuing');
 slider.css('left',sWX+'px'); //page follows finger
 // unused for now
 // previous.css('left',(sWX-width)+'px'); //moves prev with the top
 // next.css('left',(sWX+width)+'px'); //moves next with the top
 if (!((Math.abs(prevsWX)/prevsWX) == (Math.abs(sWX)/sWX))) {
 // checking to see if we are going in the same direction still
 if (sWX<0) { //moving right
 display = true;
 nextPage = currentPage + 1;
 if (nextPage > maxPage+1){ //don't display any new pages
 nextPage = maxPage+1;
 display = false;
 } else { 
 // display right page under the current page
 // hide the previous page, show the next page
 previous.addClass('hidder');
 next.css('style','').removeClass('hidder');
 display = true; 
 } 
 } 
 if (sWX>0) { //moving Left
 display = true;
 nextPage = currentPage - 1;
 if (nextPage< 0 ){ 
 nextPage = 0;
 display = false;
 } else{ 
 // display left page under the current page
 // hide the next page, show the previous page
 next.addClass('hidder'); 
 previous.css('style','').removeClass('hidder');
 display = true;
 } 
 } 
 }
 prevsWX = sWX;
 } // end of same direction check
 }); //end of sideways scroll check
 slider.bind('mouseup touchend',function(ev){ 
 slider.unbind('mousemove touchmove mouseup touchend');
 display = false;
 // console.log('finished');
 if(mobile){
 ev = ev.originalEvent.touches[0];
 scrollbarV.fadeOut();
 scrollbarH.fadeOut();
 }
 if (scrollDirection ==1 && slideOn==true){ // only perform horizontal changes
 // sX = ev.pageX;
 // var ultimate = initX-sX;
 var distance = Math.abs(initX-sX);
 if (nextPage < 1){ 
 // can't go past beginning
 distance = 0; 
 }
 if (nextPage > maxPage){ 
 // can't go past end
 distance = 0; 
 }
 if (sWX == 0){
 // do nothing
 } else if ( sWX>0 ) { //are we moving the page to the left?
 if(distance>minWidth){ 
 // move to new page if we moved 25% of the width
 slider.stop(true,true).animate({'left':width+'px'},animTime/2,'easeOutBounce',function(){
 previous.css('left','0').addClass('active');
 next.addClass('hidder');
 slider.removeClass('active').addClass('hidder').attr('style','');
 currentPage = nextPage;
 localStorage.setItem('currentPage',JSON.stringify(currentPage));
 }); 
 }else{ //move back into place. Rehide the next/prev page afterwards
 slider.stop(true,true).animate({'left':0+'px'},100,'easeOutBounce',function(){
 });
 } // end of left check
 } else if ( sWX<0 ){ //are we moving the page to the right?
 if(distance>minWidth){ 
 // move to new page if we moved more than 25%
 slider.stop(true,true).animate({'left':-width+'px'},animTime/2,'easeOutBounce',function(){
 next.css('left','0').addClass('active');
 slider.removeClass('active').addClass('hidder').attr('style','');
 previous.addClass('hidder');
 currentPage = nextPage;
 localStorage.setItem('currentPage',JSON.stringify(currentPage));
 });
 }else{ //move back into place. Rehide the next/prev page afterwards
 slider.stop(true,true).animate({'left':0+'px'},100,'easeOutBounce',function(){
 }); 
 } //end of right check
 }// finished horizontal checks
 }
 }); //end mouse/touch end
 }); //end mouse/touch start
 } // end of two checks (remover:animated & slideOn)
 }); //end return each(?) function?
 } //end 'inner' function
 })(jQuery); // end it all...

Some simple caveats:

currentPage is a variable used to hold where a person was when they last opened the publication (the publication can be stored for offline reading).

A typical page consists of a background graphic and scrollable region which contains content. The function is called in this manner, where article-wrapper is a class applied to a page. Each page is required to have a unique ID, article1-article'n':

$('.article-wrapper').jScrollTouch();

There is also an overlay which behaves differently. It contains scrollable content but it shouldn't move horizontally.

The end result for the project is an online publishing system that is free, similar to WordPress but dedicated to creating a more formal publication rather than a blog. The immediate next step is better layout options through CSS and inclusion of HTML5's canvas animations.

Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Sep 11, 2011 at 16:32
\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

Style belongs to CSS

Avoid styling code in JavaScript, especially large blocks like this one:

 scrollbarV.css({ 'display':'none',
 'position':'absolute',
 'width':'5px',
 'height':scrollbarV_length+'px',
 'left':width-10+'px',
 'top':0,'background':'black',
 'border':'1px white solid',
 '-webkit-border-radius':'5px',
 'opacity':'0.9' });

Such styling details belong to a CSS file. In JavaScript (and HTML) you should use appropriate class attributes instead.

Use consistent indentation

The indentation here is pretty nutty:

 $('.article-wrapper').not('.active').addClass('hidder');
 if(previous.is(':animated')){
 previous.stop(true, true).removeClass('hidder');
 }
 if (next.is(':animated')){
 next.stop(true, true).removeClass('hidder');
 }

Should have been:

 $('.article-wrapper').not('.active').addClass('hidder');
 if (previous.is(':animated')) {
 previous.stop(true, true).removeClass('hidder');
 }
 if (next.is(':animated')) {
 next.stop(true, true).removeClass('hidder');
 }

Btw the CSS class "hidder" looks odd too. Did you mean "hidden" instead?

The messy indentation style is rampant in the posted code, making it extremely difficult to read. Please review and correct everywhere.

Chaining with else-if

It seems these two condition cannot be true at the same time:

 if (nextPage < 1){ 
 // can't go past beginning
 distance = 0; 
 }
 if (nextPage > maxPage){ 
 // can't go past end
 distance = 0; 
 }

... in which case it would be better to use else if for the second.

Open-sourcing

An open-source project should be impeccable, to attract users and contributors. This code looks really messy, which gives the impression as if you don't care, and if you don't care, why should contributors care? Messy code is unlikely to attract supporters, but very easy to fix, so I urge you to do that.

answered Sep 5, 2015 at 1:03
\$\endgroup\$
0
3
\$\begingroup\$

First off, if you have lines of code that are commented out, you should probably either uncomment them, or remove them. Commented out code is an eyesore.

Secondly, some of your variable names are, not good. For example, I have no idea what the variable sX, sWX, or lpos do. Variable names should be not too long, but not too short, and be as descriptive as possible about the purpose of the variable.

A lot of your spacing is odd as well. For example, the block of code from your code below:

scrollbarH.css({ 'display':'none',
 'position':'absolute',
 'height':'5px',
 'width':scrollbarH_length+'px',
 'top':scroller.innerHeight()-7+'px',
 'left':0,'background':'black',
 'border':'1px white solid',
 '-webkit-border-radius':'5px',
 'opacity':'0.9'});

Would become the below block of code:

scrollbarH.css({ 
 'display': 'none',
 'position': 'absolute',
 'height': '5px',
 'width': scrollbarH_length + 'px',
 'top': scroller.innerHeight() - 7 + 'px',
 'left': 0,
 'background': 'black',
 'border': '1px white solid',
 '-webkit-border-radius': '5px',
 'opacity': '0.9'
});

Generally, you should also have space between your operators. It usually makes expressions much easier to read as well.

Finally, I'd recommend finding a code style, and then writing in that style. I usually follow this style guide for how I write my Javascript code.

janos
113k15 gold badges154 silver badges396 bronze badges
answered Jul 5, 2015 at 2:21
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.