Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix #4574, #4575 feat(visualization): group default secondary metrics #5131

Merged
merged 1 commit into from Jun 10, 2021
Merged

fix #4574, #4575 feat(visualization): group default secondary metrics #5131

merged 1 commit into from Jun 10, 2021

Conversation

robhudson
Copy link
Member

@robhudson robhudson commented Apr 29, 2021 •
edited

Screenshot 2021-05-25 at 16-32-37 On Off about welcome Experiment – Analysis Experimenter

@robhudson robhudson force-pushed the grouped-metrics branch 2 times, most recently from a8bb566 to 90ff30b Compare May 17, 2021 17:25
@robhudson robhudson force-pushed the grouped-metrics branch 4 times, most recently from 21bb821 to 311862f Compare June 1, 2021 18:37
@robhudson robhudson force-pushed the grouped-metrics branch 2 times, most recently from 2eac472 to f79d53c Compare June 2, 2021 22:04
@robhudson robhudson marked this pull request as ready for review June 2, 2021 22:09
@robhudson robhudson changed the title Group search and usage metrics fix #4574, #4575 feat(visualization): group default secondary metrics Jun 2, 2021
emtwo
emtwo reviewed Jun 4, 2021
View reviewed changes
Copy link
Contributor

@emtwo emtwo left a comment

Choose a reason for hiding this comment

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

Code looks great overall! Thanks for your patience with this!

I left some comments but they're all minor. I'm going to test it out with a few different experiments before approving but likely next week.

app/experimenter/visualization/api/v3/models.py Outdated Show resolved Hide resolved
app/experimenter/visualization/api/v3/models.py Outdated
"active_hours",
]
GROUPED_METRICS = {
# TODO: Once front-end changes are done, give this a try...
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can file a separate issue for this

app/experimenter/visualization/api/v3/models.py Show resolved Hide resolved
app/experimenter/visualization/api/v3/models.py Show resolved Hide resolved
app/experimenter/visualization/api/v3/models.py Outdated Show resolved Hide resolved
app/experimenter/nimbus-ui/src/components/PageResults/TableVisualizationRow/index.tsx Outdated Show resolved Hide resolved
app/experimenter/nimbus-ui/src/components/PageResults/TableVisualizationRow/index.tsx Outdated Show resolved Hide resolved
app/experimenter/nimbus-ui/src/components/PageResults/TableVisualizationRow/index.tsx Show resolved Hide resolved
app/experimenter/nimbus-ui/src/lib/visualization/utils.tsx Outdated Show resolved Hide resolved
app/experimenter/nimbus-ui/src/components/PageResults/index.tsx Outdated Show resolved Hide resolved
@emtwo
Copy link
Contributor

emtwo commented Jun 7, 2021

In visually reviewing the PR, I noticed a couple of things:

  1. I think it should say "Show Other Metrics" instead of "Show other". Just a little clearer (screenshot below)
    Screen Shot 2021-06-07 at 12 54 26 PM

  2. This one is a little bigger of an issue. In the screenshot below, the left navbar shows the default metrics but they are collapsed by default. This is confusing but also the bigger issue is that clicking on one of the links in the nav on the left does not scroll you down to where the metric is. Perhaps the metrics should not show up in the nav on the left if they are collapsed. It would also be nice to have subheadings in the left nav to show what the groups are instead of them all being under "default metrics"
    Screen Shot 2021-06-07 at 12 54 43 PM

I realize the changes for #2 might be a lot. I think what we could do is make the collapse open by default for now so we can merge this PR. Then in a follow-up PR, the sidebar changes can be made and then the collapse would be changed to closed by default. @robhudson let me know what you think.

emtwo
emtwo approved these changes Jun 10, 2021
View reviewed changes
Copy link
Contributor

@emtwo emtwo left a comment

Choose a reason for hiding this comment

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

this is amazing!!! thanks so much and am excited to see it live
image

@robhudson robhudson merged commit 258574f into main Jun 10, 2021
@robhudson robhudson deleted the grouped-metrics branch June 10, 2021 16:37
@robhudson robhudson mentioned this pull request Jun 17, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emtwo emtwo emtwo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@robhudson @emtwo