analytics icon indicating copy to clipboard operation
analytics copied to clipboard

Adds ability to select graph detail (interval)

Open Vigasaurus opened this issue 2 years ago • 6 comments

Changes

Recreation of #1366 rebased on top of the newly updated #1364 with several improvements & fixes (again, this should only be merged after #1364)

  • Performs the same as #1366 did at the end of discussions
  • Now only reloads the graph view on changing interval as opposed to entire page
  • More cleanly loads and unloads graph views (keeps top stats in view unless they are changing)
  • Stores & maintains interval at the graph-level instead of polluting the query object with value only used by one report

I could potentially see some benefit here in keeping the interval prop in the query schema to allow for sharing a link with the interval pre-selected, but I figured the better UX of not reloading the entire page on updating the interval was a better tradeoff.

Tests

  • [X] Automated tests have been added

Changelog

  • [ ] Entry has been added to changelog

Documentation

  • [x] Docs have been updated

Dark mode

  • [X] The UI has been tested both in dark and light mode

Screenshots

image image image image

Vigasaurus avatar Dec 31 '21 07:12 Vigasaurus

BundleMon

Files updated (1)
Status Path Size Limits
:white_check_mark: static/js/dashboard.js
297.78KB (+2.01KB +0.68%) -
Unchanged files (6)
Status Path Size Limits
:white_check_mark: static/css/app.css
515.19KB -
:white_check_mark: static/js/app.js
12.13KB -
:white_check_mark: static/js/embed.host.js
5.58KB -
:white_check_mark: static/js/embed.content.js
5.06KB -
:white_check_mark: tracker/js/plausible.js
748B -
:white_check_mark: static/js/applyTheme.js
314B -

Total files change +2.01KB +0.24%

Final result: :white_check_mark:

View report in BundleMon website ➡️


Current branch size history | Target branch size history

bundlemon[bot] avatar Dec 31 '21 07:12 bundlemon[bot]

This is mostly up to date and functional, I just need to update the interactions with the new periods available (all time & YTD), which I'll add in soon.

Vigasaurus avatar Apr 13 '22 08:04 Vigasaurus

Nice. No rush, I still have some work to do on the main graph selections with imported data.

ukutaht avatar Apr 13 '22 09:04 ukutaht

main graph selections with imported data

I can't say I'm super surprised there were some issues with that heh - ~~I haven't been able to get the import fully working on my self hosted box nor my dev environment yet to test that out fully. The issue on my self hosted box is a bit weird honestly - the oban job for the import has just been stuck on available for a while~~

Update: I was able to get the import all sorted on my self-hosted box, it definitely also needs some work with regards to this PR (some of the intervals don't quite work).

Vigasaurus avatar Apr 13 '22 09:04 Vigasaurus

Alright @ukutaht - this should be all good to go now. It had some issues with the imported data handling of the new week interval (naturally), and needed updating for the new periods on the FE which should both be sorted. I did also end up finding and fixing various UI/UX edge cases + got some code cleanup done.

I must say, I'm really not happy with the default formatting that gets forced on the longer fragments in imported.ex and timeseries.ex but I guess it is what it is (maybe those strings can be pulled into a separate variable earlier in the function to not have that deep of nesting? - I'm unsure of how that works with Ecto).

Vigasaurus avatar Apr 19 '22 12:04 Vigasaurus

Could it be considered to add the "graph detail" as a dropdown in the upper right corner, which is similar to what the user is already familiar with from the "Top Sources" section?

I made a quick prototype for it which can be seen here in Figma

With this approach, we don't confuse the user by adding a new icon that:

  • They don't know what does, and will need to learn their way around.
  • Will most likely be hidden amongst the other icons in the existing space.

https://user-images.githubusercontent.com/10897924/176861875-8131ec29-f7ae-4aff-bbea-47aedf516317.mp4

.

lambaek avatar Jul 01 '22 08:07 lambaek

@Vigasaurus Any update on this? Would be really helpful since we have a strong weekly pattern in our data 🙏

segments-tobias avatar Oct 10 '22 12:10 segments-tobias

Thanks for this extensive work, @Vigasaurus! We'll be reviewing it this week and start some testing in our staging environment this week.

vinibrsl avatar Oct 26 '22 20:10 vinibrsl

Sounds great! I haven't looked at this much since it was first completed, so do let me know if there are any new incompatibilities or issues with anything new you discover and I'd be happy to help resolve them (though I have no doubt y'all's expanded team can handle them now too 😀).

I'd also be happy to pick up, continue, or hand off the partially completed comparisons work down the line too - but @ukutaht we can discuss that later if you'd like.

Vigasaurus avatar Oct 26 '22 21:10 Vigasaurus

Sounds great! I haven't looked at this much since it was first completed, so do let me know if there are any new incompatibilities or issues with anything new you discover and I'd be happy to help resolve them (though I have no doubt y'all's expanded team can handle them now too 😀).

I'd also be happy to pick up, continue, or hand off the partially completed comparisons work down the line too - but @ukutaht we can discuss that later if you'd like.

Thanks for your cooperation. There is one bug we couldn't figure out. When hovering the Tooltip component inside TopStats row this exception is thrown:

image
Uncaught TypeError: current is undefined

The above error occurred in the <Tooltip> component:
    in Tooltip (created by TopStats)
    in TopStats (created by LineGraph)
    in div (created by LineGraph)
    in div (created by FadeIn)
    in FadeIn (created by LineGraph)
    in div (created by LineGraph)
    in LineGraph (created by Context.Consumer)
    in withRouter(LineGraph) (created by VisitorGraph)
    in div (created by FadeIn)
    in FadeIn (created by VisitorGraph)
    in div (created by VisitorGraph)
    in div (created by _default)
    in _default (created by VisitorGraph)
    in VisitorGraph (created by Historical)
    in div (created by Historical)
    in Historical (created by _class)
    in _class (created by Dashboard)
    in Dashboard (created by _class)
    in _class (created by Context.Consumer)
    in withRouter(_class) (created by Router)
    in Route (created by Router)
    in Router (created by BrowserRouter)
    in BrowserRouter (created by Router)
    in Router
    in ErrorBoundary

React will try to recreate this component tree from scratch using the error boundary you provided, ErrorBoundary.

We tried debugging but no luck. Could you please help us figure it out?

vinibrsl avatar Oct 27 '22 14:10 vinibrsl

Absolutely - I'll take a look today.

Vigasaurus avatar Oct 28 '22 16:10 Vigasaurus

@vinibrsl I was able to find it!

That was a surprisingly easy fix for something that was relatively difficult to find haha - such a small amount of a diff.

Do let me know if anything else comes up!

Also, I do realize in using this a bit that it opens up the door for a better UX if a week-long date picker option is also available, to allow for the same behavior as months and days in the graph (click to view) for weeks as well.

Vigasaurus avatar Oct 31 '22 06:10 Vigasaurus

@Vigasaurus thanks for the prompt response!

I checked out your Elixir changes only, and it's compatible with the current/master version of the dashboard. This means we can ship API support for intervals without any breaking changes. I'm co-authoring any commits I make so you are credited accordingly.

Here's my plan for releasing this feature:

  • [x] Deploy the back-end with a separate PR (#2417)
  • [x] Make a separate PR with JS changes unrelated to intervals
  • [x] Create a feature flag for intervals
  • [ ] Re-review the original PR (that will have fewer changes due 1 and 2)
  • [ ] Deploy dashboard changes under a feature flag

If it's not too much to ask, could you please help reviewing #2417?

vinibrsl avatar Nov 02 '22 20:11 vinibrsl

This pull request is now ready to be reviewed, @plausible/developers. Back-end support for intervals is already in production, and it's playing perfectly with the master version of the dashboard ✨

The last step is to merge and deploy this pull request, which includes the dashboard changes to support intervals. I added a feature flag called intervals that shows the "Graph detail" button conditionally, so we can release gradually.

image

Shout-out to @Vigasaurus who developed this highly-requested feature :) thank you again, and sorry this is taking so long! We want to be as safe as we can when releasing big changes.

vinibrsl avatar Nov 16 '22 20:11 vinibrsl

wow nice work @vinibrsl! i need this enabled for my site so i can play around! :)

metmarkosaric avatar Nov 16 '22 20:11 metmarkosaric

This pull request is now ready to be reviewed, @plausible/developers. Back-end support for intervals is already in production, and it's playing perfectly with the master version of the dashboard ✨

The last step is to merge and deploy this pull request, which includes the dashboard changes to support intervals. I added a feature flag called intervals that shows the "Graph detail" button conditionally, so we can release gradually.

image

Shout-out to @Vigasaurus who developed this highly-requested feature :) thank you again, and sorry this is taking so long! We want to be as safe as we can when releasing big changes.

Waiting anxiously to test this on staging :) From the screenshot I can think of two improvements to test:

  1. Switch the positions of the CSV export button and the graph detail selector
  2. Maybe always showing the actual interval is better than 'Graph detail'. If it shows 'Daily ↓', it's more informative immediately and the arrow would be a good indicator that there are more options.

Both of these are just ideas that we could experiment with. I don't want to give any further UI feedback without playing around with it interactively.

ukutaht avatar Nov 17 '22 11:11 ukutaht

I think I got every code review comment covered. Thanks all for your inputs 🙌

I pushed one extra commit reviewing the overall user experience, including smooth transitions, dark mode support, fixed a bug where the interval input would blink when changing intervals, etc. I also implemented @ukutaht's suggestion of displaying the selected interval replacing the "Graph detail" label.

Screen Shot 2022-11-18 at 23 34 18

I tested the CSV export, and it is not taking intervals into account. We should definitely fix that. Can we do that in a separate pull request, as I'm guessing that would be mainly back-end code? I feel like this pull request is starting to grow again, and I want to avoid that.

If you have some time, please give this branch a try. Seeding the database may help. Let me know if there are any other UX improvements I can make, or bugs you find.

vinibrsl avatar Nov 19 '22 02:11 vinibrsl

If you have some time, please give this branch a try. Seeding the database may help. Let me know if there are any other UX improvements I can make, or bugs you find.

i don't know how to give this branch a try but i'd love to test this feature and send any feedback in order not to delay things too much. for this purpose, it would be nice if we can get these types of features that make important visual changes to the UI in staging itself so everyone can test there with for instance our live demo data.

metmarkosaric avatar Nov 21 '22 09:11 metmarkosaric

it would be nice if we can get these types of features that make important visual changes to the UI in staging itself so everyone can test there with for instance our live demo data.

We don't have live demo data mirroring from prod->staging yet. Staging only has some fake data. Not great for UI testing because the dashboard doesn't 'feel real'. We can implement the live mirroring at some point next year for some public dashboards that are good for testing.

Correct me if I'm wrong @vinibrsl but the best way to test this right now is in production. It will be deployed behind a feature flag which means it can be enabled for your personal site @metmarkosaric for testing before we enable it for the public. We can use the same approach for comparisons, funnels and other new features that don't modify existing UI too much.

ukutaht avatar Nov 21 '22 14:11 ukutaht

it would be nice if we can get these types of features that make important visual changes to the UI in staging itself so everyone can test there with for instance our live demo data.

We don't have live demo data mirroring from prod->staging yet. Staging only has some fake data. Not great for UI testing because the dashboard doesn't 'feel real'. We can implement the live mirroring at some point next year for some public dashboards that are good for testing.

Correct me if I'm wrong @vinibrsl but the best way to test this right now is in production. It will be deployed behind a feature flag which means it can be enabled for your personal site @metmarkosaric for testing before we enable it for the public. We can use the same approach for comparisons, funnels and other new features that don't modify existing UI too much.

You're right! The best path for assessing this feature is in production, as we don't have real data patterns in staging yet. It is under a feature flag, so we should be good to go. Once it's deployed I'll add your user IDs to the feature flag.

vinibrsl avatar Nov 21 '22 19:11 vinibrsl

nice one, thank you!

metmarkosaric avatar Nov 21 '22 19:11 metmarkosaric

Amazing work, @Vigasaurus! We'll release this feature incrementally during the week 💙

vinibrsl avatar Nov 22 '22 12:11 vinibrsl

Awesome - thanks! For the record - I did some quick testing (including with production demo data via API route rewrites) and it seems to function super well from what I can see! Really was quite interesting to see y'all's more robust review and iteration cycle on PRs now, honestly appreciated a lot of the comments on my (albeit somewhat old) code throughout. Excited to see this feature roll out!

Vigasaurus avatar Nov 23 '22 05:11 Vigasaurus