-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Open
Labels
BugSomething is broken. Auto assigns a BugZero manager.Something is broken. Auto assigns a BugZero manager.ExternalAdded to denote the issue can be worked on by a contributorAdded to denote the issue can be worked on by a contributorReviewingHas a PR in reviewHas a PR in reviewWeeklyKSv2KSv2
Description
Coming from #78915...
Problem
The SettlementButton component is quite complex (and important!), and I struggled to review changes made to part of it due to its complexity.
Solution
Let's clean up this important component! In order:
- Add automated UI tests covering the business logic for this component.
customTextandsecondLineTextalone have many permutations of conditions and expected results. These can be captured and documented by automated tests, which will help developers understand this component and more confidently make changes. - Remove manual memoization to make it compile with React Compiler. There are 25 instances of this warning:
This component hasn't been memoized by React Compiler.
Reason: Existing memoization could not be preserved
This manual memoization is making the component more complex and longer.
- (exploration needed) Break it down into smaller, distinct components (by report type?) (i.e: using composition). It looks like there's significant differences in behavior for invoices vs expense reports vs IOU reports. Rather than having all three wrapped up in one component, let's DRY the common parts into a shared hook and keep the other parts separate, using the correct component for each type.
Reactions are currently unavailable
Metadata
Metadata
Labels
BugSomething is broken. Auto assigns a BugZero manager.Something is broken. Auto assigns a BugZero manager.ExternalAdded to denote the issue can be worked on by a contributorAdded to denote the issue can be worked on by a contributorReviewingHas a PR in reviewHas a PR in reviewWeeklyKSv2KSv2