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

feat: add countly metrics #309

Merged
merged 12 commits into from
Dec 5, 2022
Merged

feat: add countly metrics #309

merged 12 commits into from
Dec 5, 2022

Conversation

SgtPooki
Copy link
Member

@SgtPooki SgtPooki commented Oct 11, 2022

Threw together a quick banner and some logic

image

includes consent notification banner
@SgtPooki SgtPooki requested a review from a team as a code owner October 11, 2022 21:09
@SgtPooki SgtPooki linked an issue Oct 11, 2022 that may be closed by this pull request
1 task
@SgtPooki SgtPooki marked this pull request as draft October 11, 2022 21:09
@SgtPooki
Copy link
Member Author

@juliaxbow any thoughts on simple improvements I can make to the banner without hurting the UX?

@SgtPooki
Copy link
Member Author

@juliaxbow sent a mock in slack:
image

@SgtPooki SgtPooki changed the title feat: add countly metrics [WIP] feat: add countly metrics Nov 29, 2022
@SgtPooki SgtPooki marked this pull request as ready for review November 29, 2022 21:18
@whizzzkid
Copy link
Contributor

@SgtPooki are there designs available for this? I feel we're pushing down the actual content a bit too much. Maybe a modal box or how about we put this at the end of the page and just link the notices on the top?

@juliaxbow
Copy link
Collaborator

juliaxbow commented Nov 29, 2022

@whizzzkid alternative design proposal was to do a banner at the top (gdpr notice-esque). Since we have somewhat of an endless scroll, I think many users will fail to see the opt-in if it's at the bottom. Thoughts on a banner? OK with a modal box as well but this seemed less intrusive while still visible.

@whizzzkid details for banner only; other designs not agreed upon:
Banner Fill: rgba(246, 248, 251, 1)
Text: IBM Plex Sans, Regular 16, rgba(7, 58, 83, 1)
Link text (for link in paragraph and "Decline" button): IBM Plex Sans, Regular 16; rgba(107, 196, 206, 1)
"Agree" Button Details: padding: 10px 12px; gap: 10px; stroke: 1pt, rgba(107, 196, 206, 1)

Mockup (Banner):
Loading Page (With Notice Banner)

In Figma:
https://www.figma.com/file/lB6Jby1QCTLMhVG0x7TqFW/IPFS-Design-Work?node-id=2%3A660&t=23UWHv9xCE7vDOy7-0

Slack convo:
https://filecoinproject.slack.com/archives/C03KQ8MC62Y/p1665523248891319

@SgtPooki
Copy link
Member Author

@SgtPooki are there designs available for this? I feel we're pushing down the actual content a bit too much. Maybe a modal box or how about we put this at the end of the page and just link the notices on the top?

yea I don't think we should post it at the bottom at all. it will never get accepted. It should be at the top as a minor inconvenience for those who don't choose to opt-in. I don't think it's super obtrusive at all, but I like the new mock Julia put up that reduces the vertical space it takes up

whizzzkid
whizzzkid previously approved these changes Nov 30, 2022
Copy link
Contributor

@whizzzkid whizzzkid left a comment

Choose a reason for hiding this comment

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

nits

src/metrics.ts Show resolved Hide resolved
src/metrics.ts Outdated Show resolved Hide resolved
src/metrics.ts Outdated Show resolved Hide resolved
src/global.d.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@tinytb
Copy link

tinytb commented Dec 1, 2022

Personally, I would be wary of clicking a button that says "Contribute to the improvement of IPFS" on it.

It makes me think that clicking the button will take me to GitHub where I'll directly contribute code to improve IPFS; or maybe take me a survey form where I'll have to answer a bunch of questions.

I think there is a reason that most analytics consent forms have simple, innocuous button text. Just "OK" or "Agree".

SgtPooki and others added 2 commits December 1, 2022 13:09
Co-authored-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com>
whizzzkid
whizzzkid previously approved these changes Dec 1, 2022
Copy link
Contributor

@whizzzkid whizzzkid left a comment

Choose a reason for hiding this comment

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

re-applying stale review

@SgtPooki
Copy link
Member Author

SgtPooki commented Dec 3, 2022

FYI @juliaxbow @whizzzkid @tinytb comments addressed and metric updates have been made

highlights:

  • You can bring the banner back to adjust consent settings (necessary vs all) via a little cookie button unobtrusively above the table
  • banner looks good on mobile and desktop (Julia to confirm…)
  • Disagreeing still sets consent for necessary metrics, but displays text informing the user about this, with a link to what that means on countly.
  • The app is still usable if users don't click agree or disagree, but no consent is given and thus no metrics are sent (you can confirm in network tab)
  • correct consent is set when loading from localStorage (either all or necessary)

You can also test this directly on fleek now that I've got it integrated and publishing previews from PRs: https://bafybeidgyaifyhbyytawkm4lzmwwgvqlaz35h4odb5o6mspdyv4wtuxkgi.on.fleek.co/

Copy link
Contributor

@whizzzkid whizzzkid 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 better ❤️

@SgtPooki SgtPooki merged commit c727202 into master Dec 5, 2022
@SgtPooki SgtPooki deleted the 306-add-metrics branch December 5, 2022 18:25
@SgtPooki SgtPooki self-assigned this Dec 5, 2022
@BigLep
Copy link

BigLep commented Dec 5, 2022

@SgtPooki : a couple of things:

  1. Do you think the difference between "all" and "necessary" is clear for end users? I know I was given links, but one was to a github issue and one is to countly page. On the surface I wasn't clear how to compare the two.
  2. What information are users going to be most concerned about that are in "all" but are not in "necessary".
  3. Is there a way for a user to change this later?

@lidel
Copy link
Member

lidel commented Dec 6, 2022

Filled some follow-up issues:

@SgtPooki
Copy link
Member Author

Do you think the difference between "all" and "necessary" is clear for end users? I know I was given links, but one was to a github issue and one is to countly page. On the surface I wasn't clear how to compare the two.

Not entirely with this implementation. We will be working on addressing that, see #340 and ipfs/ipfs-gui#125

What information are users going to be most concerned about that are in "all" but are not in "necessary".

I will be updating ipfs/ipfs-gui#125 to clarify the metrics that are sent with "all"

Is there a way for a user to change this later?

We are going to allow opt-out. further discussion happening at ipfs/ipfs-gui#125 and ipfs-shipyard/ignite-metrics#2

github-actions bot pushed a commit that referenced this pull request Apr 17, 2023
## 1.0.0 (2023-04-17)

### Features

* /ipns/ check ([#313](#313)) ([10a5c13](10a5c13))
* add countly metrics ([#309](#309)) ([c727202](c727202))
* add cthd.icu ([#294](#294)) ([a2e0102](a2e0102))
* add https://ipfs.czip.it ([#374](#374)) ([f3dde51](f3dde51))
* add https://ipfs.joaoleitao.org ([#323](#323)) ([787f131](787f131))
* add ipfs.1-2.dev ([#169](#169)) ([c764ad6](c764ad6))
* add ipfs.drink.cafe ([#116](#116)) ([cc84899](cc84899))
* add ipfs.jpu.jp ([#348](#348)) ([b52b8c7](b52b8c7))
* add ipfs.litnet.work ([#222](#222)) ([1fd2e68](1fd2e68))
* add ipfs.pinksheep.whizzzkid.dev ([#326](#326)) ([d40a2c1](d40a2c1))
* add ipfs.soul-network.com ([#389](#389)) ([57fe04d](57fe04d))
* add nftstorage.link gateway ([#204](#204)) ([e588108](e588108))
* add Onion Gateway (TOR)  fzdqwfb5ml56oadins5jpuhe6ki6bk33umri35p5kt2tue4fpws5efid.onion ([#212](#212)) ([01ff12f](01ff12f))
* add w3s.link gateway ([#288](#288)) ([000a26f](000a26f))
* country flags ([#96](#96)) ([84a31fe](84a31fe))
* Create CODEOWNERS ([#283](#283)) ([b62b41c](b62b41c))
* Deleted https://ipfs.czip.it ([#393](#393)) ([77d67a4](77d67a4))
* Implementing Trustless Server Checks ([#310](#310)) ([4a2c926](4a2c926))
* improved Origin detection via img tag ([#117](#117)) ([8407e80](8407e80))
* improved origin isolation check ([#148](#148)) ([abd4c1c](abd4c1c))
* Introducing Service Worker For Cache Busting ([#357](#357)) ([0536782](0536782))
* new gateway https://ipfs.tayfundogdas.me/ipfs ([#321](#321)) ([9d5b552](9d5b552))
* remove ipfs.foxgirl.dev ([#155](#155)) ([15fd028](15fd028))
* remove smartsignature.io ([#146](#146)) ([77b45b6](77b45b6))
* subdomain gateways and Origin isolation check ([#78](#78)) ([afcbffa](afcbffa))
* update geoip dataset (2020-10-13) ([4187738](4187738))
* update geoip dataset (2020-10-13) ([#115](#115)) ([782b66b](782b66b))
* use typescript ([#194](#194)) ([10958e6](10958e6))

### Bug Fixes

* ⏪ Reverting [#323](#323): ipfs.joaoleitao.org ([#394](#394)) ([b5bb34c](b5bb34c))
* **ci:** add empty commit to fix lint checks on master ([3ae6aa0](3ae6aa0))
* **ci:** skip test if no code changed ([#210](#210)) ([7d6d628](7d6d628))
* cleanup entries missing DNS A record ([#180](#180)) ([2b7ad30](2b7ad30))
* do not redirect IPNS checks ([#325](#325)) ([79bb51d](79bb51d))
* flag column and new ipfs-geoip dataset ([#319](#319)) ([f5fc723](f5fc723))
* metrics consent prompt location and styling ([#353](#353)) ([e709f2b](e709f2b))
* npm start should work without prior cmds ([#307](#307)) ([7ebe2e5](7ebe2e5))
* opt-out from redirects done by browser extension ([6dd5f51](6dd5f51))
* origin typo ([#200](#200)) ([d198abb](d198abb))
* **origin:** confirm paths redirect to subdomain ([#156](#156)) ([b837a35](b837a35))
* remove heart ([#332](#332)) ([f61ec84](f61ec84))
* update ipfs.ivoputzer.xyz gateway entry ([#152](#152)) ([4b760d9](4b760d9))
* update metrics collection banner to modal with management toggle settings ([#373](#373)) ([d925b36](d925b36))
* update redirect opt-out symbol to final version ([efd5dbf](efd5dbf))

### Trivial Changes

* **deps-dev:** bump aegir from 36.2.3 to 37.5.5 ([#305](#305)) ([1d62fc3](1d62fc3))
* **deps-dev:** bump aegir from 37.5.5 to 37.5.6 ([#316](#316)) ([d3cd9bd](d3cd9bd))
* **deps-dev:** bump browserslist from 4.19.3 to 4.21.4 ([#295](#295)) ([7850071](7850071))
* **deps-dev:** bump eslint-config-ipfs from 2.1.0 to 3.1.1 ([#300](#300)) ([f1bce91](f1bce91))
* **deps-dev:** bump eslint-config-ipfs from 3.1.1 to 3.1.2 ([#315](#315)) ([0506c19](0506c19))
* **deps-dev:** bump ipfs from 0.62.1 to 0.64.2 ([#296](#296)) ([d47e503](d47e503))
* **deps-dev:** bump ipfs from 0.64.2 to 0.65.0 ([#322](#322)) ([b400349](b400349))
* **deps-dev:** bump typescript from 4.6.2 to 4.8.3 ([#293](#293)) ([c56afa1](c56afa1))
* **deps-dev:** bump typescript from 4.8.3 to 4.8.4 ([#304](#304)) ([899b4fc](899b4fc))
* **deps:** bump @dutu/rate-limiter from v1.3.0 to v1.3.1 ([#299](#299)) ([5e598e8](5e598e8))
* **deps:** bump aegir from 36.1.3 to 36.2.3 ([#202](#202)) ([8fa5851](8fa5851))
* **deps:** bump jpeg-js from 0.4.3 to 0.4.4 ([#253](#253)) ([65f99f3](65f99f3))
* **deps:** ipfs-http-client@58.0.1 ([#308](#308)) ([1bcda7c](1bcda7c))
* improve submission/PR info ([#119](#119)) ([a238f3f](a238f3f))
* improved security notes ([#151](#151)) ([5893f35](5893f35)), closes [#148](#148) [/github.com//pull/151#issuecomment-857193370](https://github.com/ipfs//github.com/ipfs/public-gateway-checker/pull/151/issues/issuecomment-857193370)
* ipfs-geoip v5 ([0d8091e](0d8091e))
* ipfs-geoip@8.0.0 ([c4e8180](c4e8180))
* readme cleanup ([798777e](798777e))
* remove dead hostnames ([#280](#280)) ([e861280](e861280))
* remove expired domains ([#179](#179)) ([16c9985](16c9985))
* remove ipfs-zod.tv ([#234](#234)) ([6f79a80](6f79a80))
* removed birds-are-nice.me ([#173](#173)) ([ff2e05c](ff2e05c)), closes [#172](#172)
* removing my gateway for now ([#335](#335)) ([cf61e68](cf61e68))
* style formatting and linting fixes ([#366](#366)) ([a81d48b](a81d48b))
* Update .github/workflows/stale.yml [skip ci] ([5fc4a68](5fc4a68))
* update readme with link to fleek ([#337](#337)) ([3dc5dbe](3dc5dbe))
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.

Add pageview metrics to countly
6 participants