Skip to content

Minor label fixes to TimelineBarChart #674

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

evan-schiewe
Copy link

@evan-schiewe evan-schiewe commented Apr 22, 2025

This PR implements two small labeling fixes to the TimelineBarChart component:

  1. Format the mouseover labels as "HH:MM" strings. They are currently displayed a decimals in units of hours, which is not particularly readable especially for a single day time period.
  2. Fixes a small bug in the X-axis label generation for the month time period that generated ordinals like "21th", "22th", etc.

Important

Fixes mouseover label format to "HH:MM" and corrects ordinal suffixes for month day labels in TimelineBarChart.vue.

  • Behavior:
    • Mouseover labels in TimelineBarChart.vue now display time as "HH:MM" instead of decimal hours.
    • Fixes ordinal suffixes for day labels in month view to correctly display "1st", "2nd", "3rd", etc.
  • Functions:
    • Adds toOrdinalSuffix() function in TimelineBarChart.vue to generate correct ordinal suffixes.
    • Updates tooltip label callback in chartOptions() to format time as "HH:MM".

This description was created by Ellipsis for 29a3af4. You can customize this summary. It will automatically update as commits are pushed.

Copy link
Contributor

@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.

Caution

Changes requested ❌

Reviewed everything up to 7785217 in 1 minute and 50 seconds. Click for details.
  • Reviewed 49 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/visualizations/TimelineBarChart.vue:74
  • Draft comment:
    Great use of Intl.PluralRules for ordinal suffixes. Consider removing or updating the FIXME comment if the month is now determined via the given timeperiod start.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. src/visualizations/TimelineBarChart.vue:117
  • Draft comment:
    Tooltip label formatting now converts fractional hours to HH:MM correctly. Confirm that context.parsed.y is always defined as expected.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking the PR author to confirm that a variable is always defined as expected, which falls under the rule of not asking the author to confirm their intention or ensure behavior. It doesn't provide a specific suggestion or point out a clear issue.
3. src/visualizations/TimelineBarChart.vue:80
  • Draft comment:
    Ordinal generation refactor looks good, but the FIXME about using timeperiod_start to determine the month remains. Consider addressing it to ensure correct day count for non-standard month starts.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_j5wZRILnNXFDOcRo

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@0xbrayo
Copy link
Member

0xbrayo commented May 1, 2025

@ellipsis-dev review

Copy link
Contributor

@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.

Important

Looks good to me! 👍

Reviewed everything up to 29a3af4 in 1 minute and 24 seconds. Click for details.
  • Reviewed 45 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/visualizations/TimelineBarChart.vue:76
  • Draft comment:
    Good fix for month ordinal labels. Consider moving Intl.PluralRules and ordinalsEnUS outside of the computed property to avoid re-instantiation on each re-render.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
2. src/visualizations/TimelineBarChart.vue:116
  • Draft comment:
    The tooltip callback cleanly converts fractional hours to HH:MM. Ensure rounding edge cases (e.g., minute rounding to 60) are sufficiently tested.
  • Reason this comment was not posted:
    Confidence changes required: 10% <= threshold 50% None
3. src/visualizations/TimelineBarChart.vue:72
  • Draft comment:
    The FIXME comment about needing access to the timeperiod start for the month labels is now obsolete. Remove it if no longer needed.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
4. src/visualizations/TimelineBarChart.vue:116
  • Draft comment:
    For strict 'HH:MM' formatting, consider zero-padding the hours as well (e.g. using padStart) so that single-digit hours appear as '01', '02', etc.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_6XRg4SGPu8eX82Qn

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

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.

2 participants