Skip to content

Conversation

@dlaliberte
Copy link
Contributor

@dlaliberte dlaliberte commented Mar 30, 2021

Prerequisites:

  • Unless it is a hotfix it should be merged against the dev branch
  • Branch is up-to-date with the branch to be merged with, i.e. dev
  • Build is successful
  • Code is cleaned up and formatted

Summary

Adds IndicatorCorrelationChart, used on the new Indicator Correlation Page.

open TODOs

  • update text to final revision
  • design review

@dlaliberte dlaliberte changed the title Dlaliberte/correlation5 Indicator Correlation Page Mar 30, 2021
@netlify
Copy link

netlify bot commented Mar 30, 2021

Preview link ready!

Built with commit 1b41405

https://deploy-preview-878--cmu-delphi-covidcast.netlify.app

dlaliberte and others added 4 commits March 30, 2021 19:32
Previous impl used a map, here we walk both lists
directly.  Arguably this is a bit easier to read than
two reduce statements.
@sgratzl sgratzl added the enhancement New feature or request label Mar 31, 2021
@dlaliberte
Copy link
Contributor Author

image

@dlaliberte
Copy link
Contributor Author

image

@dlaliberte
Copy link
Contributor Author

I haven't figured out how to use the lag in each of the indicator charts, like I was doing in the SensorCard.

@dlaliberte
Copy link
Contributor Author

I can't seem to run eslint, even though it is installed locally.

@dlaliberte dlaliberte marked this pull request as ready for review March 31, 2021 17:25
@sgratzl
Copy link
Member

sgratzl commented Apr 12, 2021

as discussed I reduced the correlation plot size on desktops:

image

@sgratzl sgratzl requested a review from chinandrew April 12, 2021 13:23
@chinandrew
Copy link
Contributor

Screenshot from 2021-04-12 10-05-11

Should we update this to say "Mouse over the R_2..." instead of "Click on ..."?

@chinandrew
Copy link
Contributor

Screenshot from 2021-04-12 10-30-16

This about box should be above the pllots

@sgratzl
Copy link
Member

sgratzl commented Apr 12, 2021

This about box should be above the pllots

done

@chinandrew
Copy link
Contributor

chinandrew commented Apr 14, 2021

Few requests after conversations with the team.

  • The primary one is reversing the polarity of the lags. e.g. what is +5 now becomes -5.
  • Adding "earlier/later" text to the plot hover (which will then need to be flipped with (a))
  • A few copy edits
  • I've also added some edits to the indicator text doc, (Apr 14th edits)

Screenshot from 2021-04-14 13-50-29

@sgratzl
Copy link
Member

sgratzl commented Apr 15, 2021

  • The primary one is reversing the polarity of the lags. e.g. what is +5 now becomes -5.

done

@sgratzl
Copy link
Member

sgratzl commented Apr 15, 2021

Few requests after conversations with the team.

I mean all are done

@chinandrew
Copy link
Contributor

chinandrew commented Apr 15, 2021

Awesome, thanks. Few last minute edits, then I think we'll be good to go.

  • when using earlier/later text, use the absolute value of the # days. e.g. this should say 16 instead of -16
    Screenshot from 2021-04-15 12-02-22
  • Looks like this might have an extra whitespace before "8 days later"
    Screenshot from 2021-04-15 07-24-16
  • For the x axis label on the lagged correlations plot, add the indicator name, so "lag (days)" -> "indicator name lag (days)"
  • Another copy edit on the doc, again the edits from today.

EDIT: one more nit:

  • the grey box here extends a bit too far down and looks like its intruding on the plot
    Screenshot from 2021-04-15 12-59-19

@sgratzl
Copy link
Member

sgratzl commented Apr 15, 2021

* For the x axis label on the lagged correlations plot, add the indicator name, so "lag (days)" -> "_indicator name_ lag (days)"

I assume you mean something different than:
image

Copy link
Contributor

@chinandrew chinandrew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit: :shipit: :shipit:

@sgratzl sgratzl merged commit f8a2795 into dev Apr 15, 2021
@sgratzl sgratzl deleted the dlaliberte/correlation5 branch April 15, 2021 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants