Allow multi hover on metric graphs (v2) #1077
Conversation
|
Your preview environment pr-1077-bttf has been deployed. Preview environment endpoints are available at: |
| scaleLinear<number>({ | ||
| domain: [ | ||
| 0, | ||
| Math.max(...data.map((d) => Math.min(d.v * 2, d.v + (d.s ?? 0) * 2))), |
There was a problem hiding this comment.
Do we need to support binomial here again?
There was a problem hiding this comment.
Not specifically no. d.v (datapoint's value) will capture what we need for a binomial variable.
| <> | ||
| {type === "binomial" ? ( | ||
| <div className={styles.val}> | ||
| <em>n</em>: {Math.round(d.v)} | ||
| {smoothBy === "week" && ( | ||
| <sub style={{ fontWeight: "normal", fontSize: 8 }}> smooth</sub> | ||
| )} | ||
| </div> | ||
| ) : ( | ||
| <> | ||
| <div className={styles.val}> | ||
| {method === "sum" ? `Σ` : `μ`}:{" "} | ||
| {formatConversionRate(type, d.v as number)} | ||
| {smoothBy === "week" && ( | ||
| <sub style={{ fontWeight: "normal", fontSize: 8 }}> smooth</sub> | ||
| )} | ||
| </div> | ||
| {"s" in d && method === "avg" && ( | ||
| <div className={styles.secondary}> | ||
| {`σ`}: {formatConversionRate(type, d.s)} | ||
| {smoothBy === "week" && ( | ||
| <sub style={{ fontWeight: "normal", fontSize: 8 }}> smooth</sub> | ||
| )} | ||
| </div> | ||
| )} | ||
| <div className={styles.secondary}> | ||
| <em>n</em>: {Math.round(d.c)} | ||
| </div> | ||
| </> | ||
| )} | ||
| <div className={styles.date}>{date(d.d as Date)}</div> | ||
| </> | ||
| ); |
There was a problem hiding this comment.
The tooltip contents are pretty difficult to parse, not sure if it's because of the existing Datapoint and how it has properties like v, s, etc. I think this function probably warrants an explanation comment that maybe includes an example of the output.
There was a problem hiding this comment.
decided to push this out a bit since it will probably change again once we address ratio metrics.
| <> | ||
| {type === "binomial" ? ( | ||
| <div className={styles.val}> | ||
| <em>n</em>: {Math.round(d.v)} |
There was a problem hiding this comment.
As we discussed offline, technically you don't need to round this as it could show the smoothing better, but it could also be confusing for some users, so I think rounding makes sense.
There was a problem hiding this comment.
yeah, i think rounding makes most sense from a UI perspective. It's weird to say that on Wednesday we had 3.62 users convert.
This is a rework of the following PR:
#1075
It uses an external state rather than internal messaging between graphs. It also bases external state on date rather than X position, which allows for graphs that my have different X axes. It also enables northstar hover sync.