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

Add year to X axis of multi-year graph #2607

Merged
merged 7 commits into from
Feb 14, 2023
Merged

Conversation

idfunctor
Copy link
Contributor

Changes

Please describe the changes made in the pull request here.
This PR solves Issue #2573
It checks if the period of the graph is "all" or "custom", as the other periods make it intuitive to understand what year the data belongs to. If the period is one of those 2, it prints the year on the entries of the X axis. Does so both for viewing by date and by week.

Below you'll find a checklist. For each item on the list, check one option and delete the other.

Tests

  • Automated tests have been added
  • This PR does not require tests

Changelog

  • Entry has been added to changelog
  • This PR does not make a user-facing change

Documentation

  • Docs have been updated
  • This change does not need a documentation update

Dark mode

  • The UI has been tested both in dark and light mode
  • This PR does not change the UI

@vinibrsl
Copy link
Contributor

vinibrsl commented Jan 19, 2023

Hey, @idfunctor 👋!

Thanks for your contribution!

If I understand correctly, your commit displays the year when period is "All time" or "Custom". If "Last 30 days" is selected and it's January, for example, two years will be returned and the year won't be displayed. To make this work in different scenarios without having to check each period, I think we would need to actually check the graph labels.

The function that builds the X-axis of the graph is: assets/js/dashboard/stats/graph/visitor-graph.js:80. I was thinking we could check for multi-year scenarios with:

// Pseudo-code warning :)
// Somewhere near assets/js/dashboard/stats/graph/visitor-graph.js:80

const yearsInGraph =
  graphData.labels
  .map(date => date.split('-')[0])
  .filter((value, index, list) => list.indexOf(value) === index)

if (yearsInGraph.length > 1) {
  return specialDateFormatterWithYear()
}

Let me know your thoughts, and thanks again for your contribution!

@idfunctor
Copy link
Contributor Author

@vinibrsl Sorry for being MIA, had a lot going on last week. Thanks for clarifying how it should work, and yes I agree it should be done that way. I did try to see if I could derive a "hasMultipleYears" boolean from the code but for some reason I didn't spend much time trying to figure that out. The code you shared is sufficient for me to get it done now!

I'll get on this sometime in the next 2-3 days and request your review again.

aerosol
aerosol previously requested changes Feb 9, 2023
Copy link
Member

@aerosol aerosol left a comment

Choose a reason for hiding this comment

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

@vinibrsl
Copy link
Contributor

@vinibrsl Sorry for being MIA, had a lot going on last week. Thanks for clarifying how it should work, and yes I agree it should be done that way. I did try to see if I could derive a "hasMultipleYears" boolean from the code but for some reason I didn't spend much time trying to figure that out. The code you shared is sufficient for me to get it done now!

I'll get on this sometime in the next 2-3 days and request your review again.

No worries. Let me know if you need any help :)

@idfunctor
Copy link
Contributor Author

@vinibrsl I just pushed changes, again really sorry for the delay, lmk if anything else needs to be done

@vinibrsl vinibrsl dismissed aerosol’s stale review February 14, 2023 11:51

Requested changes were fixed

@bundlemon
Copy link

bundlemon bot commented Feb 14, 2023

BundleMon

Files updated (1)
Status Path Size Limits
static/js/dashboard.js
298.52KB (+164B +0.05%) -
Unchanged files (6)
Status Path Size Limits
static/css/app.css
515.18KB -
static/js/app.js
12.13KB -
static/js/embed.host.js
5.58KB -
static/js/embed.content.js
5.06KB -
tracker/js/plausible.js
748B -
static/js/applyTheme.js
314B -

Total files change +164B +0.02%

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

@vinibrsl vinibrsl merged commit 0db52ff into plausible:master Feb 14, 2023
@vinibrsl
Copy link
Contributor

Thank you! ✨

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.

3 participants