0
\$\begingroup\$

I feel this is deeply inelegant, but I really want the option of classical inheritance in the future. How could this be written better. FYI animation here is from GSAP if you are curious.

class Modal

    angular.element(document).ready =>
        @modalElem   = document.getElementById 'modal'
        @overlayElem = document.getElementById 'overlay'

    @isOpen = false

    @getWindowDim : ->
        return dim =
            height : window.innerWidth or document.documentElement.clientWidth
            width  : window.innerHeight or document.documentElement.clientHeight

    @openModal : (attrs) ->

        unless @isOpen

            @isOpen = true

            width   = attrs.width 
            height  = attrs.height
            content = attrs.content 

            wDim    = @getWindowDim()           

            @modalElem.style.width      = 0
            @modalElem.style.height     = 0

            @overlayElem.style.display  = 'block'

            TweenLite.to @modalElem, 0.75, 
                width       : width
                height      : height
                marginTop   : (height/-2)
                marginLeft  : (width/-2)
                opacity     : 1
                ease        : Back.easeOut
        @

    @closeModal : ->

        if @isOpen

            @isOpen = false

            TweenLite.to @modalElem, 0.33,
                width       : 0
                height      : 0
                marginTop   : 0
                marginLeft  : 0
                opacity     : 0
                ease        : Back.easeOut
                onComplete  : =>
                    @overlayElem.style.display = 'none'
        @

modal   = angular.module 'modal'

modal.directive 'openmodal', ->
    return (scope, element, attrs) ->
        element.bind 'click', ->
            Modal.openModal attrs   

modal.directive 'closemodal', ->
    return (scope, element, attrs) ->
        element.bind 'click', ->
            Modal.closeModal()      
\$\endgroup\$
2
\$\begingroup\$

Honestly, I'd recommend against creating a class. You may want classical inheritance in the future, but right now it's not necessary. Premature optimization rarely benefits anyone. Besides, if you do want classical inheritance later, it's pretty trivial to convert the code.

I'd just write it as simply as possible for now, and see what I'll actually need later:

do -> # wrap in an IIFE
  # simple local variables
  isOpen      = false
  modalElem   = null
  overlayElem = null
  modal       = angular.module 'modal'

  angular.element(document).ready ->
    modalElem   = document.getElementById 'modal'
    overlayElem = document.getElementById 'overlay'

  open = (attrs) ->
    return if isOpen # return early
    isOpen = true

    options =
      opacity:    1
      ease:       Back.easeOut
      height:     attrs.height
      width:      attrs.width
      marginTop:  attrs.height / -2
      marginLeft: attrs.width / -2

    modalElem.style.width  = 0
    modalElem.style.height = 0
    overlayElem.style.display = 'block'
    TweenLite.to modalElem, 0.75, options

  close = ->
    return unless isOpen # return early
    TweenLite.to modalElem, 0.33,
      width:      0
      height:     0
      marginTop:  0
      marginLeft: 0
      opacity:    0
      ease:       Back.easeOut
      onComplete: ->
        overlayElem.style.display = 'none'
        isOpen = false # moved this here, so it flips when the modal's fully closed


  modal.directive 'openmodal', ->
    (scope, element, attrs) ->
      element.bind 'click', -> open attrs

  modal.directive 'closemodal', ->
    (scope, element, attrs) ->
      element.bind 'click', -> close()

If you're compiling this as a single file with CoffeeScript's default IIFE wrapper, you can skip the do -> and indentation; it'll be added automatically.


If you don't want to take the approach above, here are some tips for your current code:

Using explicit return is only necessary if you want something other than the last evaluated expression to be returned. In you open/close functions, it'd be beneficial to simply return early (like I do in the code above) instead of having a big if branch.

Also, a neat trick:

getWindowDim : ->
  return dim =
    height : window.innerWidth or document.documentElement.clientWidth
    width  : window.innerHeight or document.documentElement.clientHeight

can be written as

getWindowDim : ->
  height : window.innerWidth or document.documentElement.clientWidth
  width  : window.innerHeight or document.documentElement.clientHeight

The return is implicit, there's no need for a variable assignment, and CoffeeScript will automatically make an object.

\$\endgroup\$
  • \$\begingroup\$ actually it turns out getWindowDim is not consumed at all! In your finished example, please replace height and width with the version from attrs and I will accept your answer. +1 \$\endgroup\$ – Fresheyeball Apr 15 '13 at 16:25
  • \$\begingroup\$ @Fresheyeball Whoops - fixed now \$\endgroup\$ – Flambino Apr 15 '13 at 16:33
  • \$\begingroup\$ It's a trend I don't really like in coffeescript (and ruby). People tends to write as less characters as possible and it alters the readability of the code. I'm not sure that getting rid of the returnreally helps clarity. When the returnkeyword is present, it's clear that the function is meant to return something. It might be personal, but I'm pretty sure I'm not the only one out there. \$\endgroup\$ – jackdbernier Feb 11 '14 at 3:00
  • \$\begingroup\$ @jackdbernier I know what you're saying. But it also depends on how you view the code overall. If you regard it as more functional (as you can with ruby and js/cs), variables and function calls become interchangeable; a function just is its return value - and all functions always return something, even if it's undefined. (Obviously, this looks better in languages - like ruby - where you can leave out the () in calls). Naming is key, though; nouns for pseudo-variable functions, verbs for function functions (i.e. the above should be called windowDimensions without the get prefix) \$\endgroup\$ – Flambino Feb 12 '14 at 3:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.