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

ADR 61: Displaying Stats in UTC Date Ranges #6442

Merged
merged 8 commits into from
Nov 18, 2024
Merged

Conversation

goplayoutside3
Copy link
Contributor

@goplayoutside3 goplayoutside3 commented Nov 6, 2024

Package

lib-user
lib-content

Linked Issue and/or Talk Post

Toward: #6400

Describe your changes

  • ADR 61 describes the need for clearly labeled UI components that display stats data and three scenarios where volunteers could be confused by stats displayed in dates other than their local timezone.
  • I added a visual label below the x-axis on lib-user's BarChart. This BarChart is only ever used to display user stats or groups stats, so we can assume its x-axis unit is a date range in UTC.
  • I also added a brief question + answer to the FAQ page for volunteer and zoo team reference.

How to Review

  • Please read through the ADR and comment if anything should be added or revised.
  • Run app-root locally and visit your personal stats page and/or a group stats page. The bar chart has a label below the x-axis indicating the date range is UTC.
  • The FAQ page has a new question + answer.
  • Refactoring app-project's YourStats component will be a future PR.

Checklist

PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.

General

  • Tests are passing locally and on Github
  • Documentation is up to date and changelog has been updated if appropriate
  • You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
  • FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)
  • FEM works in a mobile browser

@coveralls
Copy link

coveralls commented Nov 6, 2024

Coverage Status

coverage: 77.873%. remained the same
when pulling 6c5f0fe on stats-adr
into 958e806 on master.

@goplayoutside3
Copy link
Contributor Author

@seanmiller26 we already chatted on Slack about the x-axis label. Here are the screenshots again for reference.
desktop
mobile

Here's a screenshot of the FAQ question + answer. The text is easily revised before or after this PR is merged if needed.
faq

@goplayoutside3 goplayoutside3 requested review from mcbouslog and a team November 6, 2024 01:07
@goplayoutside3
Copy link
Contributor Author

goplayoutside3 commented Nov 6, 2024

@mcbouslog I'm requesting your code review because your familiarity with the BarChart.

@kieftrav and @shaunanoordin I've tagged @frontend for review too because of the ADR. It's a good idea to read through it for reference in case you're answering questions from volunteers about stats or debugging stats components in the future.

@mcbouslog mcbouslog self-assigned this Nov 7, 2024
Copy link
Contributor

@mcbouslog mcbouslog left a comment

Choose a reason for hiding this comment

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

ADR breaks down the topic well, I really appreciate/like the examples.

FAQ item is helpful reference 👍 .
BarChart content looks good across screens/theme.

Noting default end date (i.e. here in UserStats or GroupStats) will be UTC date, consistent with this ADR.

docs/arch/adr-61.md Outdated Show resolved Hide resolved
docs/arch/adr-61.md Outdated Show resolved Hide resolved
@github-actions github-actions bot added the approved This PR is approved for merging label Nov 15, 2024
@mcbouslog mcbouslog removed their assignment Nov 15, 2024
@goplayoutside3 goplayoutside3 merged commit 4d0cde2 into master Nov 18, 2024
7 checks passed
@goplayoutside3 goplayoutside3 deleted the stats-adr branch November 18, 2024 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This PR is approved for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants