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

refactor: make the score result message account for the selected query period #617

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

crimsonskylark
Copy link

@crimsonskylark crimsonskylark commented Oct 27, 2024

Previously the score visualization always said "total score today" regardless of what the period selected was. This PR adjusts the message text with a relative time expression where reasonable (e.g. past 30 days.)

The current behaviour remains but is extended with support for "this week", "this month" and "this year".


Important

Updates score message in Score.vue to reflect selected time period, supporting various durations like 'this week' and 'past 30 days'.

  • Behavior:
    • Updates score message in Score.vue to reflect selected time period instead of always 'today'.
    • Supports 'this week', 'this month', 'this year', and relative periods like 'past 30 days'.
  • Functions:
    • Adds selected_timeperiod computed property in Score.vue to determine time period description based on useActivityStore().query_options.timeperiod.

This description was created by Ellipsis for 47420d2. It will automatically update as commits are pushed.

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 47420d2 in 12 seconds

More details
  • Looked at 59 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. src/visualizations/Score.vue:80
  • Draft comment:
    The destructuring assignment seems incorrect. It should be:
      const [duration, span] = useActivityStore().query_options.timeperiod;
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment seems to address a potential issue with the destructuring assignment. If timeperiod is indeed an array or tuple, the suggestion is correct. However, if length is a property that returns an array or tuple, the original code might be correct. Without additional context on the structure of timeperiod, it's difficult to be certain.
    I might be missing the exact structure of timeperiod. If timeperiod is not an array or tuple, the suggestion could be incorrect. The comment assumes a specific structure without evidence from the provided code.
    Given the lack of context, it's safer to assume the comment might be incorrect unless there's strong evidence supporting the suggested change.
    Delete the comment due to insufficient evidence supporting the suggested change. The structure of timeperiod is unclear, and the comment assumes a specific structure without evidence.

Workflow ID: wflow_PiehIPADQqO9jhFr


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

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.

1 participant