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

Add a link to GLAM for the selected metric #306

Merged
merged 4 commits into from
Jan 14, 2021
Merged

Add a link to GLAM for the selected metric #306

merged 4 commits into from
Jan 14, 2021

Conversation

robhudson
Copy link
Member

No description provided.

@robhudson robhudson requested a review from wlach January 8, 2021 18:11
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 looks and works great! Just a few minor changes and we'll be good to land.

src/pages/MetricDetail.svelte Outdated Show resolved Hide resolved
src/pages/MetricDetail.svelte Outdated Show resolved Hide resolved
@robhudson robhudson requested a review from wlach January 8, 2021 22:25
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.

Looks great, thanks!

@@ -197,6 +225,15 @@
{/each}
</td>
</tr>
{#if glamUrl}
Copy link
Contributor

@wlach wlach Jan 11, 2021

Choose a reason for hiding this comment

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

Argh, sorry, did some last minute testing and it looks like this doesn't do the right thing for events (it incorrectly suggests we can view them in GLAM). Maybe the suggestion below will fix that?

Suggested change
{#if glamUrl}
{#if glamUrl && metric.type !== "event"}

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, thanks. I added a condition similar to one just above it. Let me know if that's correct. It worked locally for me.

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.

Awesome. I'm going to hold off on landing this until #312 is merged so as not to cause unnecessary merge conflicts.

@wlach wlach merged commit a51ba0e into main Jan 14, 2021
@wlach wlach deleted the 279-link-glam branch January 14, 2021 19:29
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