search

lagotto / alm-report

ALM Reports
http://almreports.plos.org/
MIT License
8 stars 3 forks source link

Feedback on visualizations #95

Closed mfenner closed 9 years ago

mfenner commented 9 years ago

General

jure commented 9 years ago

Fantastic feedback, Martin! Thanks, will get right on it.

jure commented 9 years ago

Looks like the affiliations property comes back null from the API, which is why there are no markers. Looking into it.

mfenner commented 9 years ago

Something we can do is add more checks that ENV parameters are present. The app was silently failing on me with a missing PER_PAGE parameter. Fixed now, but difficult to troubleshoot, and maybe affiliations is also related to that.

jure commented 9 years ago

If we'll do that, then it makes sense to expand on the dotenv.rb intializer, and transfer the values into a CONFIG constant, with proper types, to avoid casting at every other location that uses the ENV.

mfenner commented 9 years ago

I also meant something like the following: https://github.com/articlemetrics/alm-report/blob/master/config/initializers/dotenv.rb#L3-L4

This raises an error on application startup if a required ENV variable is missing. Feels nicer than checking an ENV variable every time we use them.

We could add typecasting to the initializer, but people might expect ENV variables to always be a string. I'm still not 100% convinced we need the extra layer of a CONFIG system, but that is of course your call.

jure commented 9 years ago

The extra layer is a must if you want to do typecasting, i.e. it's not possible to re-store strings as integers directly into ENV:

ENV["TEST"] = 2
TypeError: no implicit conversion of Fixnum into String

Which means you need some other place to store them, some other constant, which is why CONFIG would be needed here.

mfenner commented 9 years ago

Ok, makes sense. Another option - which I like a bit more than CONFIG - would be to use secrets.yml for that:

production:
  per_page: <%= ENV["PER_PAGE"].to_i %>

And then use Rails.application.secrets.per_page, which will be an integer.

jure commented 9 years ago

Hm, so we already have the ENV checking system in place, we were just missing the PER_PAGE ENV variable? Adding PER_PAGE and continuing to use .to_i doesn't sound that bad anymore then.

jure commented 9 years ago

http://f.fontdeck.com/s/css/js/test.parascope.io/24557.js not found

Fixed by including the test.parascope.io domain.

Maps map visualization doesn't show any markers

Fixed, was an issue with missing seeds in the Geocode table.

Bubble Charts make font on bubble chart axes bigger

Done.

tooltip in bubble charts: show publication date (instead of months) and doi

Not done. Jennifer wanted "Months: " (https://github.com/articlemetrics/alm-report/issues/101), @jenniferlin15 would you like publication date instead?

tooltip in bubble charts and other places: format numbers with separator for thousands, e.g. 34,128

Done.

bubble chart: use PLOS colors for journals

Done.

bubble chart end date in legend wrong? http://test.parascope.io/reports/visualizations/7 has January 2015 date

Yes, it was a bug, due to January being the 0th month for JavaScript. Fixed in https://github.com/articlemetrics/alm-report/commit/75ea07656362c2ae814a0752d253ddfffd4dc540

new Date(2014, 12, 2)
Fri Jan 02 2015 00:00:00 GMT+0100 (CET)
new Date(2014, 12-1, 2)
Tue Dec 02 2014 00:00:00 GMT+0100 (CET)

bubble chart: PubMed Central Usage Stats instead of PMC

Fixed in https://github.com/articlemetrics/alm-report/commit/cc1493f0d1b8a95017b55f238131251f9f6cf1c1

bubble chart: make y axis label horizontal

Fixed.

bubble chart: consider making bubbles smaller overall

I think they look ok, now that the chart is bigger.

image

font size of "select your source" too small compared to select box

Looks ok now? image

consider increasing height of bubble chart, e.g. for aspect ratio 3:2

Fixed. Made chart wider too.

consider black border around bubbles

Without the black borders, the circles merge into one another. I think it look good for now.

Sunburst Charts consider increasing diameter of sunburst chart to fit with size of other charts on that page

Done.

sunburst chart: make it clearer how to navigate back from detail

Added text to description in https://github.com/articlemetrics/alm-report/commit/dbb722de2a152c587e52c092dc770d6072f368c8

sunburst chart: increase font size in center

Done.