RS-20085: Only set rhtmlwidget-status to ready once images have loaded (#133)

* Remove unsued code

* Only set rhtmlwidget-status to ready once images have loaded

* Bump version

Author: JustinCCYap

DESCRIPTION

@@ -1,7 +1,7 @@
 Package: rhtmlPictographs
 Type: Package
 Title: Create Pictograph html widgets
-Version: 1.0.8
+Version: 1.0.9
 Author: Displayr <opensource@displayr.com>
 Maintainer: Displayr <opensource@displayr.com>
 Description: An R HTMLWidget that can generate single image graphics, mutli

inst/htmlwidgets/rhtmlPictographs.js

@@ -128,8 +128,9 @@ class BaseCell {
   }
 
   draw () {
-    this._draw()
-    this._generateDynamicCss()
+    return Promise.resolve()
+      .then(() => this._draw())
+      .then(() => this._generateDynamicCss())
   }
 
   setCss (cssLocation, cssAttr, cssValue) {

inst/htmlwidgets/rhtmlPictographs.js.map

@@ -538,38 +538,31 @@ class GraphicCell extends BaseCell {
       throw error // NB This is thrown because displayr wants the error too
     }
 
-    let baseImageCompletePromise = Promise.resolve()
+    const imageRenderPromises = []
     if (this.config.baseImage != null) {
       const baseImageConfig = this.config.baseImage
       const imageFactory = this.imageFactory
-      const baseImageRenderPromises = []
       enteringLeafNodes.each(function (dataAttributes) {
         const d3Node = d3.select(this)
-        baseImageRenderPromises.push(
+        imageRenderPromises.push(
           imageFactory.addBaseImageTo(d3Node, baseImageConfig, imageWidth, imageHeight, dataAttributes)
         )
       })
-      baseImageCompletePromise = Promise.all(baseImageRenderPromises).catch(imageErrorHandler)
     }
 
-    // TODO this should be tied to base image promise unconditioanlly (ie. swap lines below)
-    // let variableImageCompletePromise = baseImageCompletePromise
-    let variableImageCompletePromise = Promise.resolve()
     if (this.config.variableImage != null) {
       const variableImageConfig = this.config.variableImage
       const imageFactory = this.imageFactory
-      variableImageCompletePromise = baseImageCompletePromise.then(function () {
-        const variableImageRenderPromises = []
-        enteringLeafNodes.each(function (dataAttributes) {
-          const d3Node = d3.select(this)
-          variableImageRenderPromises.push(
-            imageFactory.addVarImageTo(d3Node, variableImageConfig, imageWidth, imageHeight, dataAttributes)
-          )
-        })
-        return Promise.all(variableImageRenderPromises).catch(imageErrorHandler)
+      enteringLeafNodes.each(function (dataAttributes) {
+        const d3Node = d3.select(this)
+        imageRenderPromises.push(
+          imageFactory.addVarImageTo(d3Node, variableImageConfig, imageWidth, imageHeight, dataAttributes)
+        )
       })
     }
 
+    const variableImageCompletePromise = Promise.all(imageRenderPromises).catch(imageErrorHandler)
+
     return variableImageCompletePromise.then(() => {
       if (this.config.tooltip) {
         enteringLeafNodes.append('svg:title')

theSrc/scripts/BaseCell.js

@@ -220,12 +220,7 @@ class ImageFactory {
           return null
         }
       })
-      .then((clipId) => {
-        const imageHandle = imageInstance.appendToSvg()
-        if (clipId) {
-          imageHandle.attr('clip-path', `url(#${clipId})`)
-        }
-      })
+      .then((clipId) => imageInstance.appendToSvg(clipId))
   }
 
   calculateAspectRatio (config) {

theSrc/scripts/GraphicCell.js

@@ -14,7 +14,6 @@ class Pictograph {
   constructor (el) {
     this.rootElement = _.has(el, 'length') ? el[0] : el
     d3.select(this.rootElement).attr(`rhtmlwidget-status`, 'loading')
-    d3.select(this.rootElement).attr(`rhtmlPictographs-status`, 'loading') // to be removed once regression testing code checks for rhtmlwidget-status
     const actualDimensions = this.getContainerDimensions()
     log.info('Pictograph() called. Container dimensions:', actualDimensions, 'element:', el)
 
@@ -42,7 +41,6 @@ class Pictograph {
         if (error.type === InsufficientContainerSizeError.type) {
           console.log(error.message)
           d3.select(this.rootElement).attr(`rhtmlwidget-status`, 'ready')
-          d3.select(this.rootElement).attr(`rhtmlPictographs-status`, 'ready') // to be removed once regression testing code checks for rhtmlwidget-status
         } else {
           console.error(`error in pictograph draw: ${error.message}`)
           console.error(error.stack)
@@ -79,7 +77,6 @@ class Pictograph {
         if (error.type === InsufficientContainerSizeError.type) {
           console.log(error.message)
           d3.select(this.rootElement).attr(`rhtmlwidget-status`, 'ready')
-          d3.select(this.rootElement).attr(`rhtmlPictographs-status`, 'ready') // to be removed once regression testing code checks for rhtmlwidget-status
         } else {
           console.error(`error in pictograph resize: ${error.message}`)
           console.error(error.stack)
@@ -497,6 +494,8 @@ class Pictograph {
       .attr('class', 'table-cell')
       .attr('transform', d => `translate(${d.x},${d.y})`)
 
+    const cellDrawingPromises = []
+
     enteringCells.each(function (d) {
       const instance = d.instance
       log.debug(`rendering entering cell`, instance)
@@ -508,11 +507,11 @@ class Pictograph {
       instance.setWidth(d.width)
       instance.setHeight(d.height)
       instance.setDynamicMargins(d.dynamicMargins)
-      instance.draw()
+
+      cellDrawingPromises.push(instance.draw())
     })
 
-    d3.select(this.rootElement).attr(`rhtmlwidget-status`, 'ready')
-    d3.select(this.rootElement).attr(`rhtmlPictographs-status`, 'ready') // to be removed once regression testing code checks for rhtmlwidget-status
+    Promise.all(cellDrawingPromises).then(() => d3.select(this.rootElement).attr(`rhtmlwidget-status`, 'ready'))
   }
 
   // TODO Duplicated code from graphicCell

theSrc/scripts/ImageFactory.js

@@ -64,6 +64,12 @@ class BaseImageType {
   get baseShapeHiding () {
     return (this.config.baseShapeScale != null) ? this.config.baseShapeScale : 1
   }
+
+  addClipId (clipId) {
+    if (clipId) {
+      this.imageHandle.attr('clip-path', `url(#${clipId})`)
+    }
+  }
 }
 
 module.exports = BaseImageType

theSrc/scripts/Pictograph.js

@@ -16,7 +16,7 @@ class CircleType extends BaseImageType {
     return Promise.resolve(1)
   }
 
-  appendToSvg () {
+  appendToSvg (clipId) {
     this.imageHandle = this.d3Node.append('svg:circle')
       .classed('circle', true)
       .attr('cx', this.containerWidth / 2)
@@ -25,7 +25,9 @@ class CircleType extends BaseImageType {
       .style('fill', this.color)
       .style('opacity', this.opacity)
 
-    return this.imageHandle
+    this.addClipId(clipId)
+
+    return Promise.resolve()
   }
 }
 

theSrc/scripts/imageTypes/base.imagetype.js

@@ -9,7 +9,7 @@ class EllipseType extends BaseImageType {
     return Promise.resolve(null)
   }
 
-  appendToSvg () {
+  appendToSvg (clipId) {
     this.imageHandle = this.d3Node.append('svg:ellipse')
       .classed('ellipse', true)
       .attr('cx', this.containerWidth / 2)
@@ -19,7 +19,9 @@ class EllipseType extends BaseImageType {
       .style('fill', this.color)
       .style('opacity', this.opacity)
 
-    return this.imageHandle
+    this.addClipId(clipId)
+
+    return Promise.resolve()
   }
 }