fix #4342: feature(visualization) - Incorporate metric metadata from Jetstream. #4599

emtwo · 2021-02-11T20:51:59Z

Because:

We want to have human-friendly metric names and detailed descriptinos

This commit:

Pulls in that data from Jetstream and uses it for labels and tooltips.

LZoog

LGTM from a quick pass but I'm wondering about the overlay bit. :) (will r+ after... but I should probably quickly check into your branch and run this locally in the morning to double check, heh)

Also, I feel like we could use a doc update somewhere since I don't think we mention/explain the interaction with Jetstream at all, but I'll let you make that call and think that's fine in a quick follow up ticket too if you think we need more than a sentence or two.

LZoog · 2021-02-11T23:29:32Z

app/experimenter/nimbus-ui/src/components/PageResults/TableMetricSecondary/index.tsx

+          <div className="d-inline-block">
+            {probeSetName}{" "}
+            {probeSetDescription && (
+              <OverlayTrigger overlay={popover} trigger="click" rootClose>


I might be missing something since I'm doing an end-of-day review, but I checked out the docs for this to get an idea of what I should be seeing, and I'm wondering why we wouldn't use the react-tooltip for consistency but with event="click" if we specifically want it to show onclick (if I'm reading their docs correctly)? Maybe we have to or want to use ReactMarkdown and it's difficult to get that working with a tooltip?

Also, I think Storybook might need some love, I think we should be seeing that pop up here but when I hover/click the info icon I'm not seeing anything.

app/experimenter/nimbus-ui/src/components/AppLayoutSidebarLocked/index.tsx

emtwo · 2021-02-12T04:11:01Z

LGTM from a quick pass but I'm wondering about the overlay bit. :) (will r+ after... but I should probably quickly check into your branch and run this locally in the morning to double check, heh)

It's true the overlay shows up on the top, I'm adding a screenshot here:

I believe it doesn't show up on storybook because of the fact that it's up top. I can probably update it to adjust the popup location dynamically so that it shows up in storybook properly.

Also, I feel like we could use a doc update somewhere since I don't think we mention/explain the interaction with Jetstream at all, but I'll let you make that call and think that's fine in a quick follow up ticket too if you think we need more than a sentence or two.

I agree this could use some docs. I'll file a follow-up since I do think it would be nice to have a bit more detail about what the relationship with Jetstream looks like.

I'm wondering why we wouldn't use the react-tooltip for consistency but with event="click" if we specifically want it to show onclick (if I'm reading their docs correctly)? Maybe we have to or want to use ReactMarkdown and it's difficult to get that working with a tooltip?

You're right, I decided to use the popover because the text sent from Jetstream is in markdown and it has clickable links so it can't disappear when you hover away. I had assumed tooltips are not clickable but now that I look at the documentation it seems like it might be possible.

I'm PTO tomorrow but I can look into those things when I'm back.

jaredlockhart

Backend looks good to me, couple ideas for some improvements but nothing critical, I'll let others review the frontend in more detail. G/j @emtwo ! 🎉

app/experimenter/visualization/api/v3/views.py

app/experimenter/visualization/tests/api/test_views.py

emtwo · 2021-02-16T23:13:45Z

@LZoog I managed to get the markdown working with ReactTooltip. Storybook looks good in the basic view for the results page. You can look at the tooltips there.

Unfortunately, I wasn't able to address the storybook issue that you screenshotted about. I think something about the way storybook renders the component, it doesn't recognize that it's hovering out of view. I couldn't find a way to make this work unfortunately.

Perhaps we can just let it be since it works fine in the basic view of storybook? What do you think?

jodyheavener

Left a comment on the clickability and prop declaration, and my only other feedback would be that there appears to be extra space at the bottom of the markdown tooltip:

This appears to be because it renders

tags, where the regular tooltip doesn't. I think we could solve this by adding something like this to the global styles:

.__react_component_tooltip > p:last-child {
  margin-bottom: 0;
}

app/experimenter/nimbus-ui/src/components/PageResults/TableMetricSecondary/index.tsx

…Jetstream. Because: * We want to have human-friendly metric names and detailed descriptinos This commit: * Pulls in that data from Jetstream and uses it for labels and tooltips.

emtwo · 2021-02-18T18:11:18Z

.__react_component_tooltip > p:last-child {
  margin-bottom: 0;
}

Thanks! I added this it looks nicer!

emtwo · 2021-02-18T18:12:07Z

@jodyheavener let me know what you think with the changes!

jodyheavener

We might want to style links in the tooltip to be lighter/white with underline since that blue over grey might be less visible, but I totally happy pushing that to a followup. Looks great!

emtwo · 2021-02-18T18:28:26Z

We might want to style links in the tooltip to be lighter/white with underline since that blue over grey might be less visible, but I totally happy pushing that to a followup. Looks great!

Thanks! That's a good point. I think if we want all tooltips to look consistent they might all need to be the same colour so that would need to be part of the follow-up to make sure it works across the board. I'll file a ticket

emtwo requested review from jaredlockhart, LZoog and jodyheavener February 11, 2021 21:00

LZoog reviewed Feb 11, 2021

View reviewed changes

jaredlockhart approved these changes Feb 12, 2021

View reviewed changes

emtwo force-pushed the emtwo/metadata branch 2 times, most recently from e1e8ee4 to 78ee0b4 Compare February 16, 2021 22:57

jodyheavener reviewed Feb 18, 2021

View reviewed changes

app/experimenter/nimbus-ui/src/components/PageResults/TableMetricSecondary/index.tsx Outdated Show resolved Hide resolved

fix #4342: feature(visualization) - Incorporate metric metadata from …

2f0021c

…Jetstream. Because: * We want to have human-friendly metric names and detailed descriptinos This commit: * Pulls in that data from Jetstream and uses it for labels and tooltips.

emtwo force-pushed the emtwo/metadata branch from 78ee0b4 to 2f0021c Compare February 18, 2021 17:52

emtwo requested a review from jodyheavener February 18, 2021 18:11

jodyheavener approved these changes Feb 18, 2021

View reviewed changes

emtwo merged commit fa7b40f into main Feb 18, 2021

emtwo deleted the emtwo/metadata branch February 18, 2021 18:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #4342: feature(visualization) - Incorporate metric metadata from Jetstream. #4599

fix #4342: feature(visualization) - Incorporate metric metadata from Jetstream. #4599

emtwo commented Feb 11, 2021

LZoog left a comment •

edited

LZoog Feb 11, 2021 •

edited

emtwo commented Feb 12, 2021

jaredlockhart left a comment

emtwo commented Feb 16, 2021

jodyheavener left a comment

emtwo commented Feb 18, 2021

emtwo commented Feb 18, 2021

jodyheavener left a comment

emtwo commented Feb 18, 2021

fix #4342: feature(visualization) - Incorporate metric metadata from Jetstream. #4599

fix #4342: feature(visualization) - Incorporate metric metadata from Jetstream. #4599

Conversation

emtwo commented Feb 11, 2021

LZoog left a comment • edited

Choose a reason for hiding this comment

LZoog Feb 11, 2021 • edited

Choose a reason for hiding this comment

emtwo commented Feb 12, 2021

jaredlockhart left a comment

Choose a reason for hiding this comment

emtwo commented Feb 16, 2021

jodyheavener left a comment

Choose a reason for hiding this comment

emtwo commented Feb 18, 2021

emtwo commented Feb 18, 2021

jodyheavener left a comment

Choose a reason for hiding this comment

emtwo commented Feb 18, 2021

LZoog left a comment •

edited

LZoog Feb 11, 2021 •

edited