[Maintenance] Migrating util.js and chartTheme.js to typescript #2009
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
ghosetuhin
changed the title
There was a problem hiding this comment.
👋 Hi @ghosetuhin ! Thanks for your first contribution! I left a couple of comments that I hope won't be too tricky for you to solve. And once they are solved I'll be happy to merge this PR :)
| redFadeEnd: string; | ||
| }; | ||
|
|
||
| let colorFades: ColorFades = { |
There was a problem hiding this comment.
💬 suggestion: this type is automatically inferred. There's no point in manually recreating it if we're not re-using it in multiple places.
| let colorFades: ColorFades = { | |
| let colorFades = { |
There was a problem hiding this comment.
Thanks for the suggestion! Made the changes!
| stroke: string; | ||
| }; | ||
|
|
||
| const baseLabelStyles: LabelStyles = { |
There was a problem hiding this comment.
💬 suggestion: same issue here
| const baseLabelStyles: LabelStyles = { | |
| const baseLabelStyles = { |
| tickLabels: LabelStyles; | ||
| }; | ||
|
|
||
| const axisBaseStyles: AxisBaseStyles = { |
There was a problem hiding this comment.
💬 suggestion: and here
| const axisBaseStyles: AxisBaseStyles = { | |
| const axisBaseStyles = { |
| }; | ||
| }; | ||
|
|
||
| export const chartTheme: ChartTheme = { |
There was a problem hiding this comment.
💬 suggestion: and here
| export const chartTheme: ChartTheme = { | |
| export const chartTheme = { |
| }; | ||
|
|
||
| export function getColorScale(name: string): string[] { | ||
| const scales: ColorScales = { |
There was a problem hiding this comment.
💬 suggestion: would be better to use an inline style here if we want to be super-strict. Although personally I wouldn't add a manual type to it. Up to you.
| const scales: ColorScales = { | |
| const scales: Record<string, string[]> = { |
There was a problem hiding this comment.
Implemented it inline. Thanks for the suggestion!
| @@ -13,23 +17,23 @@ export async function runAll(queries, cb) { | |||
| cb(data); | |||
| } | |||
|
|
|||
| export function index(data, field, mapper) { | |||
| export function index(data, field: string, mapper?: (input) => unknown) { | |||
There was a problem hiding this comment.
💬 suggestion: could be improved with generics
| export function index(data, field: string, mapper?: (input) => unknown) { | |
| export function index<T, K extends keyof T>(data: T[], field: K, mapper?: (input: T[K]) => string) { |
There was a problem hiding this comment.
Please take a look at this. I implemented it using generics this time
There was a problem hiding this comment.
the generics I implemented using the way you suggested were failing the type checks for some reason with the error Argument of type 'string' is not assignable to parameter of type 'never'. I tried playing around with it but couldn't fix it. Had to finally resort to using string as the type for some fields
| const result = {}; | ||
| data.forEach(item => { | ||
| result[mapper ? mapper(item[field]) : item[field]] = item; | ||
| }); | ||
| return result; | ||
| } | ||
|
|
||
| export function indexStack(data, fieldName, field) { | ||
| export function indexStack(data, fieldName: string, field: string) { |
There was a problem hiding this comment.
💬 suggestion: this one and also indexCashFlow (below) could be improved with generics too.
There was a problem hiding this comment.
I tried implementing it for indexCashFlow but it is not taking generic types quite well
trafico-bot
bot
added
⚠️ Changes requested
trafico-bot
bot
removed
the
⚠️ Changes requested
| @@ -13,23 +17,24 @@ export async function runAll(queries, cb) { | |||
| cb(data); | |||
| } | |||
|
|
|||
| export function index(data, field, mapper) { | |||
| const result = {}; | |||
| export function index<T>(data: T[], field: string, mapper?: (input) => string) { | |||
There was a problem hiding this comment.
Was a pretty fun problem to solve. Here you go: https://gist.github.com/MatissJanis/307e49bbeec34f2975709e0ec3bc3100 :)
There was a problem hiding this comment.
Ah, I see. Thanks for this! I implemented these changes!!
trafico-bot
bot
added
✨ Merged
Link to issue: #1483
Replaced util.js with util.ts
Replaced chart-theme.js with chart-theme.ts