-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 overview stats #17722
Conversation
5f9c5c0
to
e3f3470
Compare
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.
Overall looks good, just some suggestions to get this in line with out frontend guidelines.
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 still believe you're over-engineering it with the CSS grid (e.g. did you think of mobile? we have had many problems with the grid on dashboards, etc...), and it would be better for everyone to just do:
<div className='space-y-2'>
<div className='flex flex-col gap-2'>
<div className='w-50 sm:w-full md:w-full'>{first_one}</div>
<div className='w-50 sm:w-full md:w-full'>{second_one}</div>
</div>
<div>{third one}</div>
</div>
or something like that and call it a day.
Since this is feature flagged, I'll 🤷 now, but this will come back.
4098219
to
080c450
Compare
080c450
to
7a092bd
Compare
Problem
Continuing web analytics
Changes
Add a new query for some overview stats
Move shared parts of some queries into a common file
Fix an issue with the pathname cte when there weren't any pageview events
Add an extremely basic grid layout, which is still very much WIP
(the next step will be improving the UI)
How did you test this code?
(Behind FF) ran it