-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Another iteration of Dashboard.Live.Pages #5986
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
Conversation
This will only be necessary for Top Stats, which will be responsible for rendering the "with_imported_switch". Defaulting to `true` would mean `imports_skip_reason` always being returned under `meta` even when imported data was not requested.
|
zoldar
left a comment
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.
Other than module/file naming mentioned in the feedback, it's looking good to me!
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.
For consistency's sake, we could adjust the module name as well. Or even change that to dashboard/query_parser.ex and Plausible.Stats.Dashboard.QueryParser, respectively. When aliasing, we can always alias Plausible.Stats.Dashboard and use Dashboard.QueryParser to maintain the context, without redundant repetition of Dashboard.DashboardQueryParser. This applies to other renaming instances as well.
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.
Agreed! 3d5ed50
| <div className="mx-auto loading"> | ||
| <div /> | ||
| </div> | ||
| <div className={'group-has-[[data-phx-teleported]]:hidden'}> |
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.
Really nice improvement in UX ✨
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.
Actually this gets a bit tricky when we want to replace "Top Pages" -> "Conversion Pages" when filtering by a goal. As soon as the navigation event (let's say applying the goal filter) happens, the component instantly starts to load (optimistically, relying on the phx-navigation-loading class). During that optimistic loading we don't know whether the report title should be Conversion/Top pages. It requires a round-trip to the server to know whether a goal filter is applied or not.
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 got the green light to revert the dynamic report title change and keep it static as it was before a couple of days ago. Before we do that though, let's make sure there's a plan to overcome the issue with hooks in a deadview. Not a blocker to this PR anyways.
|
Ah, and there's a failing test case for external link fn under CE when running against a consolidated site: https://github.com/plausible/analytics/actions/runs/20778435502/job/59669814176?pr=5986 |
Changes
Tests
Changelog
Documentation
Dark mode