-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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(web-analytics): Add guess for initial host filter #18818
Conversation
Size Change: 0 B Total Size: 1.83 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
1f5ecc2
to
9686ad0
Compare
9686ad0
to
2a2adc9
Compare
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, and other than double checking the tldts comment, I don't see any blockers.
]) | ||
let propertiesResponse: unknown | ||
try { | ||
propertiesResponse = await api.get(`api/event/values/?${params}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor remark: we do have some kind of API composition system under api.ts
, freeing us from URL constants. Might be worth hooking into it? 🤷 through if you copied this from elsewhere.
@@ -5,6 +5,7 @@ import api from 'lib/api' | |||
import { RETENTION_FIRST_TIME, STALE_EVENT_SECONDS } from 'lib/constants' | |||
import { dayjs } from 'lib/dayjs' | |||
import { isNotNil } from 'lib/utils' | |||
import * as tldts from 'tldts' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This potentially adds about 100KB of JS. Might be worth double checking if so... and perhaps just importing { getSubdomain }
solves this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect that this doesn't help much, as it'll still need the big list of 3LDs etc. Will try it anyway!
const wwwOrApex = hosts.filter((host) => { | ||
const subdomain = tldts.getSubdomain(host) | ||
return subdomain === '' || subdomain === 'www' | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps these subdomains on their own hold value? Suggestion that's non blocking: rename this to a subdomains
loader, and use a selector to derive the filters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, also, nothing really blocking here, so have a ✅... but I'd still check if there's a way we don't include 100+kb of JS that'll mostly be unused. However if it just loads on the web analytics page, not the app generally, then 🤷
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
Problem
Some users have asked how to make the web analytics dashboard only show their marketing site.
Changes
Add a guess for which hosts are likely to be their marketing site. This guess is based on whether the subdomain is www. or the site is on the domain apex.
How did you test this code?
Manually