bookreader icon indicating copy to clipboard operation
bookreader copied to clipboard

DRY up Mode2Up.flipLeftToRight and Mode2Up.flipRightToLeft

Open cdrini opened this issue 5 years ago • 6 comments

These functions have very similar logic, and are largely duplicates of each other. DRY them up.

Summary of Requirements

  • [ ] Create pre-refactor unit tests for Mode2Up.flipLeftToRight and Mode2Up.flipRightToLeft
  • [ ] Mode2Up.flipLeftToRight and Mode2Up.flipRightToLeft should still exist, but call a helper function which takes more parameters and reduces code duplication
  • [ ] Update tests and ensure they still pass

cdrini avatar May 07 '20 20:05 cdrini

I can do this @cdrini I need some more details on it like how exactly you want to refactor?

imskr avatar Jul 09 '20 14:07 imskr

@cdrini is this still open? I'd like to work on it.

shubham00jain avatar Sep 29 '20 12:09 shubham00jain

@cdrini If the issue is still not resolved yet would like to take it up from here on.

diwanshu8850 avatar Sep 29 '20 12:09 diwanshu8850

@shubham00jain @diwanshu8850 are you working on this

ArunTeltia avatar Oct 02 '20 21:10 ArunTeltia

@cdrini @iisa can you please check I have refactored the code If you gave it a go I will send a pull request and start with the writing tests for this helper function

helperflipper(newIndexL, newIndexR, LeftToRight){
    this.br.refs.$brContainer.addClass("BRpageFlipping");
    const leftLeaf = this.br.twoPage.currentIndexL;
    const rightLeaf = this.br.twoPage.currentIndexR;

    const oldLeafEdgeWidthL = this.br.leafEdgeWidth(leftLeaf);
    const newLeafEdgeWidthL = this.br.leafEdgeWidth(newIndexL);
    const oldLeafEdgeWidthR = this.br.twoPage.edgeWidth - oldLeafEdgeWidthL;
    const newLeafEdgeWidthR = this.br.twoPage.edgeWidth - newLeafEdgeWidthL;

    const currWidthL = this.getPageWidth(leftLeaf);
    const currWidthR = this.getPageWidth(rightLeaf);
    const newWidthL = this.getPageWidth(newIndexL);
    const newWidthR = this.getPageWidth(newIndexR);
    const top = this.top();
    const middle = this.br.twoPage.middle;
    let leafEdgeTmpWL;
    let leafEdgeTmpL;
    const speed = this.br.flipSpeed;
    const gutter = this.br.twoPage.middle + this.gutterOffsetForIndex(newIndexL);

    if (LeftToRight) {
        leafEdgeTmpWL = gutter - currWidthL - oldLeafEdgeWidthL - newLeafEdgeWidthL;
        leafEdgeTmpL = gutter + newWidthR;
    } else {
        leafEdgeTmpWL = gutter + newWidthR;
        leafEdgeTmpL = gutter - newWidthL - leafEdgeTmpW;
    }
    const $twoPageViewEl = this.br.refs.$brTwoPageView;
    this.br.leafEdgeTmp = document.createElement('div');
    this.br.leafEdgeTmp.className = 'BRleafEdgeTmp';

    $(this.br.leafEdgeTmp).css({
        width: `${leafEdgeTmpW}px`,
        height: `${this.br.twoPage.height}px`,
        left: `${leafEdgeTmpWL}px`,
        top: `${top}px`,
        zIndex: 1000,
    }).appendTo($twoPageViewEl);

    let oldFlipFromLeafIndex;
    let oldFlipToLeafIndex;
    let newFlipFromLeafIndex;
    let newFlipToLeafIndex;

    if (LeftToRight) {
        $(this.leafEdgeL).css({
            width: `${newLeafEdgeWidthL}px`,
            left: `${leafEdgeTmpWL}px`
        });

        const left = $(this.br.prefetchedImgs[leftLeaf]).offset().left;
        const right = `${$twoPageViewEl.prop('clientWidth') - left - $(this.br.prefetchedImgs[leftLeaf]).width() + $twoPageViewEl.offset().left - 2}px`;
        $(this.br.prefetchedImgs[leftLeaf]).css({
            right: right,
            left: ''
        });
        oldFlipFromLeafIndex=leftLeaf;
        oldFlipToLeafIndex=rightLeaf;
        newFlipFromLeafIndex=newIndexL;
        newFlipToLeafIndex=newIndexR;

    } else {
        $(this.leafEdgeR).css({
            width: `${newLeafEdgeWidthR}px`,
            left: `${leafEdgeTmpWL}px`
        });
        oldFlipFromLeafIndex=rightLeaf;
        oldFlipToLeafIndex=leftLeaf;
        newFlipFromLeafIndex=newIndexR;
        newFlipToLeafIndex=newIndexL;
    }

    $(this.br.leafEdgeTmp).animate({ left: gutter }, speed, 'easeInSine');
    if (this.br.enableSearch) this.br.removeSearchHilites();

    $(this.br.leafEdgeTmp).animate({ left: `${leafEdgeTmpL}px` }, speed, 'easeOutSine');
    $(this.br.prefetchedImgs[oldFlipFromLeafIndex]).animate({ width: '0px' }, speed, 'easeInSine', () => {
        this.br.$('.BRgutter').css({ left: `${gutter - this.br.twoPage.bookSpineDivWidth * 0.5}px` });
        $(this.br.prefetchedImgs[newFlipToLeafIndex]).animate({ width: `${newWidthR}px` }, speed, 'easeOutSine', () => {
            $(this.br.prefetchedImgs[newFlipFromLeafIndex]).css('zIndex', 2);
            //jquery adds display:block to the element style, which interferes with our print css
            $(this.br.prefetchedImgs[newFlipFromLeafIndex]).css('display', '');
            $(this.br.prefetchedImgs[newFlipToLeafIndex]).css('display', '');

            $(this.leafEdgeR).css({
                // Moves the right leaf edge
                width: `${this.br.twoPage.edgeWidth - newLeafEdgeWidthL}px`,
                left: `${gutter + newWidthR}px`
            });
            $(this.leafEdgeL).css({
                // Moves and resizes the left leaf edge
                width: `${newLeafEdgeWidthL}px`,
                left: `${gutter - newWidthL - newLeafEdgeWidthL}px`
            });
            // Resizes the brown border div
            $(this.br.twoPage.coverDiv).css({
                width: `${this.coverWidth(newWidthL + newWidthR)}px`,
                left: `${gutter - newWidthL - newLeafEdgeWidthL - this.br.twoPage.coverInternalPadding}px`
            });
            $(this.br.leafEdgeTmp).remove();
            this.br.leafEdgeTmp = null;
            // $$$ TODO refactor with opposite direction flip
            this.br.twoPage.currentIndexL = newFlipFromLeafIndex;
            this.br.twoPage.currentIndexR = newFlipToLeafIndex;
            this.br.twoPage.scaledWL = newWidthL;
            this.br.twoPage.scaledWR = newWidthR;
            this.br.twoPage.gutter = gutter;

            this.br.updateFirstIndex(this.br.twoPage.currentIndexL);
            this.br.displayedIndices = [newFlipFromLeafIndex, newFlipToLeafIndex];
            this.br.pruneUnusedImgs();
            this.br.prefetch();
            this.br.animating = false;

            if (this.br.enableSearch) this.br.updateSearchHilites();

            this.setMouseHandlers();

            if (this.br.animationFinishedCallback) {
                this.br.animationFinishedCallback();
                this.br.animationFinishedCallback = null;
            }
            this.br.refs.$brContainer.removeClass("BRpageFlipping");
            this.br.textSelectionPlugin?.stopPageFlip(this.br.refs.$brContainer);
            this.br.trigger('pageChanged');
        });
    });

}

ArunTeltia avatar Oct 18 '20 08:10 ArunTeltia

Hey, I would like to work on this issue. Please assign me to it.

shreyan-invalid avatar Feb 02 '21 19:02 shreyan-invalid