[WIP] Migrate Balance.jsx -> Balance.tsx
#2329
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
View detailed bundle breakdownAdded No assets were added Removed No assets were removed 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
|
jfdoming
force-pushed
the
jfdoming/balance-typescript
branch
from
1ff4f76 to
c4cc5c9
Compare
| export function useSheetValue<T>( | ||
| binding: Binding, | ||
| onChange?: (result) => void, | ||
| ): T | null { |
There was a problem hiding this comment.
❓ question: any way we could make this smarter? For example: I see that binding can have a value. In such case we should be able to extract the type from there. Perhaps we can also extract the type from the binding itself my the name of it?
Basically I'm afraid that manual type definitions here will lead to subtle bugs later.
| export function useSelectedItems() { | ||
| return useContext(SelectedItems); | ||
| export function useSelectedItems<Item>() { | ||
| return useContext(SelectedItems) as Set<Item>; |
There was a problem hiding this comment.
🔨 warning: manual type coercions lead to subtle bugs that are hard to spot.
Could we instead change line 248 to be
const SelectedItems = createContext<Set<string>>(null);
There was a problem hiding this comment.
I see your point, but I was concerned that the different usages of SelectedItems wouldn't all be strings. I'll double check and fix if they are
| const sheetValue = useSheetValue<string>({ | ||
| name: rolloverBudget.toBudget, | ||
| value: 0, | ||
| }); |
There was a problem hiding this comment.
💭 thought: this is what I was afraid of. We manually type this as a string, but the value here is clearly a number.
There was a problem hiding this comment.
When sheetValue is used below, it's passed into parseInt, so I assumed it would be a string. Maybe I'm misunderstanding how useSheetValue works?
There was a problem hiding this comment.
For now - ignore how sheetValue is used. Focus on the three highlighted lines.
We provide value: 0 in there as the default value. And then we also manually type the return type as string. So which type will be returned? According to the typings: string. But in reality it could as well be number.
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
Still planning on coming back to address feedback, just been busy with other projects. Will un-stale this PR once. |
MatissJanis
changed the title
Balance.jsx -> Balance.tsxBalance.jsx -> Balance.tsx
|
Going to close this PR as I've had limited dev time lately and would like to focus on burning down the list of bugs/feature requests for "Splits in Rules" |
Relates to #1483