openlp-dev team mailing list archive

[Raoul Snyman <raoul@xxxxxxxxxxx>]

Review: Needs Fixing

See the inline comments.

Also, in your tests you seem to be testing a single style every time, you should be able to combine those into a single test. For example:

   it("should set the correct styles when the location is the top of the page", function(){
       Display.doEntranceTransition("0");
       expect(alertBackground.style.top).toEqual('0px');
       expect(alertBackground.style.transition).toEqual("2s linear");
       expect(alertBackground.style.height).toEqual("25%");
   });


Diff comments:

> 
> === modified file 'openlp/core/display/html/display.js'
> --- openlp/core/display/html/display.js	2019-02-05 21:26:30 +0000
> +++ openlp/core/display/html/display.js	2019-02-06 21:00:42 +0000
> @@ -372,15 +403,111 @@
>     * @param {string} text - The alert text
>     * @param {int} location - The location of the text (top, middle or bottom)
>    */
> - alert: function (text, location) {
> -  console.debug(" alert text: " + text, ", location: " + location);
> +  alert: function (text, location) {
> +    console.debug(" alert text: " + text + ", location: " + location);
> +
> +    if(text == ""){
> +      return null;
> +    }
> +
> +    var alertBackground = $("#alert-background")[0];
> +    var alertText = $("#alert")[0];
> +
> +    alertText.innerHTML = text;
> +
> +    /* Bring in the transition background */
> +    Display._transitionState = Display.doEntranceTransition(location);
> +
> +    alertBackground.addEventListener('transitionend', function(e){
> +      e.stopPropagation();
> +      if(Display._transitionState == TransitionState.EntranceTransition){
> +        alertText.style.visibility = "visible";
> +        alertText.classList.add("horizontal-scroll-animation");
> +      }else if(Display._transitionState == TransitionState.ExitTransition){
> +        Display._transitionState = TransitionState.NoTransition;
> +        alertBackground.style.visibility = "hidden";
> +        alertBackground.classList.remove("middle-exit-animation");
> +      }
> +    });
> +
> +    alertBackground.addEventListener('animationend',function(){
> +
> +      console.debug("Noticed an animation has ended. The animation state is: ", Display._animationState);
> +      if(Display._animationState == AnimationState.FadeInAnimation){
> +        alertText.style.visibility = "visible";
> +        alertText.classList.add("horizontal-scroll-animation");
> +        alertText.classList.remove("middle-entrance-animation");
> +        Display._animationState = AnimationState.ScrollingAnimation;
> +      }else if(Display._animationState == AnimationState.FadeOutAnimation){
> +        alertBackground.style.visibility = "hidden";
> +        alertBackground.classList.remove("middle-exit-animation");
> +        Display._animationState = AnimationState.NoAnimation;
> +      }else if(alertText.classList.contains("horizontal-scroll-animation")){
> +        alertText.classList.remove("horizontal-scroll-animation");
> +        Display._animationState = AnimationState.NoAnimation;
> +        Display._transitionState = Display.doExitTransition(location);
> +      }
> +
> +    });
> +
>    /*
>     * The implementation should show an alert.
>     * It should be able to handle receiving a new alert before a previous one is "finished", basically queueing it.
>     */
> -  return;
> -},
> -
> +    return location;
> +  },
> +
> +  /**
> +   * Start background entrance transition for display of alert
> +   * @param {string} location - String showing the location of the alert on screen
> +   */
> +  doEntranceTransition: function (location){
> +    var alertBackground = $("#alert-background")[0];
> +
> +    switch(location){
> +      case "0":

What does this number mean, in terms of location? Can you make an enumeration that helps us know what it is when looking at the code.

Something like:

var Location = {
    Top: "0",
    Middle: "1",
    Bottom: "2"
};

switch (location) {
  case Location.Top:
   ...

> +        alertBackground.style.top = '0';
> +        alertBackground.style.transition = "2s linear";
> +        alertBackground.style.height = "25%";
> +        break;
> +      case "1":
> +        alertBackground.style.top = ((window.innerHeight - alertBackground.clientHeight) / 2)
> +            + 'px';
> +        alertBackground.style.height = "25%";
> +        alertBackground.classList.add("middle-entrance-animation");
> +        Display._animationState = AnimationState.FadeInAnimation;
> +        break;
> +      case "2":
> +      default:
> +        alertBackground.style.bottom = '0';
> +        alertBackground.style.transition= "2s linear";
> +        alertBackground.style.height = "25%";
> +        break;
> +    }
> +    alertBackground.style.visibility = "visible";
> +    return TransitionState.EntranceTransition;
> +
> +  },
> +
> +  /**
> +   * Start background exit transition once alert has been displayed
> +   * @param {string} location - Integer showing the location of the alert on screen
> +   */
> +  doExitTransition: function(location){
> +
> +    var alertBackground = $("#alert-background")[0];
> +
> +    if(location == "0" || location == "2"){

I need some room to breathe.... add some spaces here between your brackets and braces. For example:

    if (location == "0" || location == "2") {

> +      alertBackground.style.height = "0%";
> +      alertBackground.style.transition = '2s linear';
> +    }else if(location == "1"){

Please put your "else" statements on the next line, it's difficult to see them when they're right next to a brace like this.

   }
   else if (location == "1") {

> +      alertBackground.classList.add("middle-exit-animation");
> +      alertBackground.style.height = "0%";
> +      Display._animationState = AnimationState.FadeOutAnimation;
> +    }
> +
> +    return TransitionState.ExitTransition;
> +  },
>    /**
>     * Add a slides. If the slide exists but the HTML is different, update the slide.
>     * @param {string} verse - The verse number, e.g. "v1"


-- 
https://code.launchpad.net/~ic90/openlp/animated-alerts/+merge/362833
Your team OpenLP Development is subscribed to branch lp:~openlp-dev/openlp/webengine-migrate.