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

fix: showing of UncategorizedNotification #571

Merged

Conversation

BelKed
Copy link
Contributor

@BelKed BelKed commented May 9, 2024

this.total is in seconds, not milliseconds



Screenshot 2024-05-09 at 06 02 52

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 7d12afd in 31 seconds

More details
  • Looked at 14 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. src/components/UncategorizedNotification.vue:33
  • Draft comment:
    The change from milliseconds to seconds for the comparison in isVisible method is correct based on the context provided. This ensures that the notification visibility logic is consistent with the unit of measurement for total.
  • Reason this comment was not posted::
    Confidence changes required: 0%
    The PR description mentions that this.total is in seconds, not milliseconds, and the change in the code reflects this understanding by changing the comparison from 60 * 60 * 1000 to 60 * 60. This change seems correct based on the description and the context provided in the code. The total is derived from uncategorizedDuration[1], which is likely in seconds as no conversion to milliseconds is evident in the code. The change ensures that the comparison is consistent with the unit of measurement for total.

Workflow ID: wflow_UWo4i7QKm2EvLJvS


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

11 days left in your free trial, upgrade for $20/seat/month or contact us.

Copy link

codecov bot commented May 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 25.60%. Comparing base (d3a22c2) to head (7d12afd).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #571   +/-   ##
=======================================
  Coverage   25.60%   25.60%           
=======================================
  Files          27       27           
  Lines        1613     1613           
  Branches      273      273           
=======================================
  Hits          413      413           
  Misses       1174     1174           
  Partials       26       26           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ErikBjare
Copy link
Member

Nice find! 🏅

@ErikBjare ErikBjare merged commit 1b1209d into ActivityWatch:master May 9, 2024
8 checks passed
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.

Categorisation Helper error (stuck on Loading...)
2 participants