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.

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

Fix #1143: Add SQL example using main_1pct #1162

Merged
merged 1 commit into from
Feb 10, 2021
Merged

Conversation

robhudson
Copy link
Member

@robhudson robhudson commented Feb 4, 2021

Updated modal with telemetry SQL tab selected...
Screenshot_2021-02-09 tls_delegated_credentials_time_until_ready_ms GLAM(1)

@robhudson robhudson requested a review from wlach February 4, 2021 22:30
Copy link
Contributor

@wlach wlach 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 great stuff.

Can you link to the cookbook somewhere in the modal? I don't think we should assume it's generally known (I keep on telling people about it).

src/components/SqlModal.svelte Outdated Show resolved Hide resolved
src/components/SqlModal.svelte Outdated Show resolved Hide resolved
@robhudson robhudson requested a review from wlach February 8, 2021 21:52
Copy link
Contributor

@wlach wlach left a comment

Choose a reason for hiding this comment

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

In general looks good! Can you show some screenshots as well?

src/components/SqlModal.svelte Outdated Show resolved Hide resolved
src/stringTemplates/desktop-glam.tpl Outdated Show resolved Hide resolved
src/stringTemplates/desktop-telemetry.tpl Outdated Show resolved Hide resolved
href="https://docs.telemetry.mozilla.org/cookbooks/main_ping_exponential_histograms.html">
Visualizing Percentiles of a Main Ping Exponential Histogram</a>
for more on the use of this query.
</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the screenshot, I'm wondering if it would look better if we pasted this text into the SQL statement as a comment. It looks a little weird to have this just below the ... :

@robhudson
Copy link
Member Author

Squashed and added screenshot

Copy link
Contributor

@wlach wlach left a comment

Choose a reason for hiding this comment

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

Sorry for the nitpicking, but what do you think about moving the copy/paste icon into the text area? Similar to what's done on dtmo:

image
https://docs.telemetry.mozilla.org/cookbooks/main_ping_exponential_histograms.html#getting-client-level-data

href="https://docs.telemetry.mozilla.org/cookbooks/main_ping_exponential_histograms.html">
Visualizing Percentiles of a Main Ping Exponential Histogram</a>
for more on the use of this query.
</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the screenshot, I'm wondering if it would look better if we pasted this text into the SQL statement as a comment. It looks a little weird to have this just below the ... :

@robhudson
Copy link
Member Author

robhudson commented Feb 9, 2021

Sorry for the nitpicking, but what do you think about moving the copy/paste icon into the text area? Similar to what's done on dtmo

I did consider that initially but it would mean refactoring the way I'm showing the SQL which is currently just in a textarea. But since you bring it up it sounds like that would be worthwhile to spend extra time on that. I changed it in this code from a button at the bottom since the telemetry SQL is so long and it is not longer visible at the bottom.

It looks a little weird to have this just below the ... :

I could just get rid of the ellipsis and reword it so it's not necessary. I think the link works well there.

@wlach
Copy link
Contributor

wlach commented Feb 9, 2021

Sorry for the nitpicking, but what do you think about moving the copy/paste icon into the text area? Similar to what's done on dtmo

I did consider that initially but it would mean refactoring the way I'm showing the SQL which is currently just in a textarea. But since you bring it up it sounds like that would be worthwhile to spend extra time on that. I changed it in this code from a button at the bottom since the telemetry SQL is so long and it is not longer visible at the bottom.

Not sure how hard it would be! If it's not too much trouble I would try that.

It looks a little weird to have this just below the ... :

I could just get rid of the ellipsis and reword it so it's not necessary. I think the link works well there.

That seems like a reasonable solution to me.

@robhudson
Copy link
Member Author

Thanks for all the reviews, @wlach!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants