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: 340 - updates to metrics collection to use modal ux #359

Closed
wants to merge 6 commits into from

Conversation

0xDanomite
Copy link
Contributor

@0xDanomite 0xDanomite commented Jan 7, 2023

@SgtPooki closing this PR in preference of #373 after the formatting cleanup

@0xDanomite 0xDanomite requested a review from a team as a code owner January 7, 2023 03:12
src/index.html Outdated
Comment on lines 11 to 12
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/tachyons@4.11.1/css/tachyons.min.css"
integrity="sha256-XiJ+PedljEmPP2VaQzSzekfCZdPr0fpqmh9dY6kpsuQ=" crossorigin="anonymous">
Copy link
Member

Choose a reason for hiding this comment

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

we should prioritize imports in javascript instead of managing in html

src/index.html Outdated
<link href="styles.css?v=0.4" type="text/css" rel="stylesheet"/>
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/tachyons@4.11.1/css/tachyons.min.css"
integrity="sha256-XiJ+PedljEmPP2VaQzSzekfCZdPr0fpqmh9dY6kpsuQ=" crossorigin="anonymous">
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/ipfs-css@0.13.1/ipfs.css"
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Comment on lines +23 to +24
<img src="https://ipfs.io/ipfs/QmTqZhR6f7jzdhLgPArDPnsbZpvvgxzCZycXK7ywkLxSyU?filename=ipfs-logo.3ea91a2f.svg"
alt="IPFS" height="50px" width="117.5px" />
Copy link
Member

Choose a reason for hiding this comment

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

please use git add -p to ensure you're not committing your own formatting changes

@@ -1,30 +1,38 @@
<!DOCTYPE html>
Copy link
Member

Choose a reason for hiding this comment

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

I can't tell what's legitimately changed in this file versus what the formatting changes are. If you want to update formatting, we should do so in a separate PR

Copy link
Member

Choose a reason for hiding this comment

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

function loadCountly (): void {
bannerToggle?.addEventListener('click', bannerToggleEventHandler)
metricsModalToggle?.addEventListener('click', metricsModalToggleEventHandler)
Countly.init({
Copy link
Member

Choose a reason for hiding this comment

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

we could probably remove all the non-DOM related stuff and replace it with an import and proper usage of ignite-metrics

body, html {
box-sizing: border-box;
text-align: center;
body,
Copy link
Member

Choose a reason for hiding this comment

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

separate formatting changes and code changes.

Copy link
Member

Choose a reason for hiding this comment

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

see #366.

@SgtPooki
Copy link
Member

Testing the changes on the preview build look good except for one thing:

Metrics are disabled upon page load:

image

we could probably address this issue and a few of my comments on this PR in another issue, #367

@tinytb
Copy link

tinytb commented Jan 18, 2023

Replaced by #373

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.

4 participants