chenejac / VIVOTestMigrationJIRA

0 stars 0 forks source link

VIVO-1950: Google visualization libraries not loading - sparklines broken #1839

Closed chenejac closed 3 years ago

chenejac commented 3 years ago

Benjamin Gross (Migrated from VIVO-1950) said:

VIVO makes copious use of Google visualization libraries. The method for loading these libraries changed at some point. See [https://developers.google.com/chart/interactive/docs/basic_load_libs#update-library-loader-code] for details.

 

 

The result is that sparkline graphs don't appear. It is not clear to me if this is the sole problem... but the issue appears to affect older versions of VIVO as well. See CU Experts (v1.9.x)

 

chenejac commented 3 years ago

Benjamin Gross said:

I should add... there is i18n sprint relevance to this, since the broken visualization javascript seems to break the language selector menu, as well.

!out.gif!

 

chenejac commented 3 years ago

Andrew Woods said:

Seems like this will also warrant patch releases for previously effected versions of VIVO.

chenejac commented 3 years ago

Brian Lowe said:

Try this around line 277 of personPublicationSparklineContent.ftl

 

Instead of drawPubCountVisualization(sparklineImgTD);

try:

google.charts.load('current', { 
 callback: function() { drawPubCountVisualization(sparklineImgTD) },          packages: ['bar', 'corechart', 'table', 'imagesparkline']
});

 

chenejac commented 3 years ago

Benjamin Gross said:

Levels of fixes for this issue

  1. Implement Brian's fix above, which appears to fix the functionality but adds a couple warnings to the console about the library already being loaded

  2. Intermediate fix where we check within Freemarker (?) if the library is already loaded, if so, don't execute the loading logic again.

  3. Track down the nested library calls and figure out how to call it only once

chenejac commented 3 years ago

Benjamin Gross said:

https://github.com/vivo-project/VIVO/pull/216

chenejac commented 3 years ago

Andrew Woods said:

For clarity, it appears that you have gone with option #1 from the three options mentioned in the comment above.

chenejac commented 3 years ago

Andrew Woods said:

Works as expected. Suggest holding off on this merge until https://github.com/vivo-project/VIVO/pull/215 has been merged.

chenejac commented 3 years ago

Andrew Woods said:

Resolved with: https://github.com/vivo-project/VIVO/commit/2122254cdba6465137cbc2889aee0ef83ebc329e