-
Notifications
You must be signed in to change notification settings - Fork 16
chore: removed leaderboard types from configs and refactor data display #1396
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: main
Are you sure you want to change the base?
Conversation
…displays Signed-off-by: Efren Lim <[email protected]>
Signed-off-by: Efren Lim <[email protected]>
Signed-off-by: Efren Lim <[email protected]>
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.
Pull Request Overview
This PR refactors the leaderboard data display architecture by removing type metadata from configuration files and creating specialized, self-contained display components. The changes move toward a more component-based approach where each display type encapsulates its own formatting and trend logic.
Key Changes
- Removed
dataType,decimals, andhideTrendproperties fromLeaderboardConfiginterface - Created specialized display components (integer, decimal, duration, timestamp) that replace the generic numeric and time-duration displays
- Embedded trend calculation logic within each display component instead of using a separate trend-display component
- Added wrapper components (longest-running, codebase-size) for displays that need trend hidden
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| leaderboard.types.ts | Removed type metadata properties from LeaderboardConfig interface |
| leaderboard-helpers.ts | Added copyright header to new helper file |
| top-100-organizations.config.ts | Updated to use IntegerDataDisplay instead of NumericDataDisplay |
| top-100-contributors.config.ts | Updated to use IntegerDataDisplay instead of NumericDataDisplay |
| small-teams-massive-output.config.ts | Updated to use IntegerDataDisplay instead of NumericDataDisplay |
| resolution-rate.config.ts | Updated to use DecimalDataDisplay instead of NumericDataDisplay |
| most-active-organizations.config.ts | Updated to use IntegerDataDisplay instead of NumericDataDisplay |
| most-active-contributors.config.ts | Updated to use IntegerDataDisplay instead of NumericDataDisplay |
| longest-running.config.ts | Updated to use LongestRunningDataDisplay wrapper component |
| focused-teams.config.ts | Updated to use IntegerDataDisplay instead of NumericDataDisplay |
| fastest-responders.config.ts | Updated to use DurationDataDisplay instead of TimeDurationDisplay |
| fastest-mergers.config.ts | Updated to use DurationDataDisplay instead of TimeDurationDisplay |
| commit-activity.config.ts | Updated to use IntegerDataDisplay instead of NumericDataDisplay |
| codebase-size.config.ts | Updated to use CodebaseSizeDataDisplay wrapper component |
| table-row.vue | Simplified to pass full data object to display components instead of individual properties |
| trend-display.vue | Removed; trend logic moved into individual display components |
| timestamp.vue | New component for displaying timestamp data with embedded trend |
| time-duration.vue | Removed; replaced by duration.vue and timestamp.vue |
| numeric.vue | Removed; replaced by integer.vue and decimal.vue |
| longest-running.vue | New wrapper component that hides trend for timestamp display |
| integer.vue | New component for integer data with embedded trend calculation |
| duration.vue | New component for duration data with embedded trend calculation |
| decimal.vue | New component for decimal data with embedded trend calculation |
| codebase-size.vue | New wrapper component that hides trend for integer display |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Formats a number to a numeric string with up to 2 units | ||
| * Example: 1,000,000 |
Copilot
AI
Nov 17, 2025
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.
The comment says "Formats a number to a numeric string with up to 2 units" which is misleading. The function actually formats numbers with thousand separators (e.g., "1,000,000") or shortens large numbers (e.g., "1.2M"), not "units".
Consider updating to: "Formats a number with thousand separators or shorthand notation for large numbers"
| * Formats a number to a numeric string with up to 2 units | |
| * Example: 1,000,000 | |
| * Formats a number with thousand separators or shorthand notation for large numbers | |
| * Example: 1,000,000 or 1.2M |
frontend/app/components/modules/leaderboards/components/data-displays/integer.vue
Show resolved
Hide resolved
| const sign = trend.value >= 0 ? '+' : '-'; | ||
|
|
||
| if (isLargeNumber.value) { | ||
| return formatNumberShort(trend.value); |
Copilot
AI
Nov 17, 2025
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.
When the trend value is large (> 1000000), the sign is missing from the formatted trend value. Line 114 returns formatNumberShort(trend.value) directly without preserving the sign that was calculated on line 111.
This should be:
if (isLargeNumber.value) {
return `${sign}${formatNumberShort(Math.abs(trend.value))}`;
}This ensures the sign is consistently displayed for large trend values, just like it is for smaller values on line 117.
| return formatNumberShort(trend.value); | |
| return `${sign}${formatNumberShort(Math.abs(trend.value))}`; |
frontend/app/components/modules/leaderboards/components/data-displays/decimal.vue
Outdated
Show resolved
Hide resolved
frontend/app/components/modules/leaderboards/components/data-displays/integer.vue
Outdated
Show resolved
Hide resolved
… normal rows in config Signed-off-by: Efren Lim <[email protected]>
In this PR
Ticket
IN-850