-
-
Notifications
You must be signed in to change notification settings - Fork 119
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 in1
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%
<= threshold50%
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 by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
@ellipsis-dev review |
There was a problem hiding this 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 in1
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%
<= threshold50%
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%
<= threshold50%
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%
<= threshold50%
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%
<= threshold50%
None
Workflow ID: wflow_6XRg4SGPu8eX82Qn
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
This PR implements two small labeling fixes to the TimelineBarChart component:
Important
Fixes mouseover label format to "HH:MM" and corrects ordinal suffixes for month day labels in
TimelineBarChart.vue
.TimelineBarChart.vue
now display time as "HH:MM" instead of decimal hours.toOrdinalSuffix()
function inTimelineBarChart.vue
to generate correct ordinal suffixes.label
callback inchartOptions()
to format time as "HH:MM".This description was created by
for 29a3af4. You can customize this summary. It will automatically update as commits are pushed.