Skip to content
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.

Sign up for GitHub

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

Fix mobile balance modal colors #3492

Merged
merged 5 commits into from
Sep 26, 2024
Merged

Fix mobile balance modal colors #3492

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2d8584c
Fix mobile balance modal colors
joel-jeremy Sep 23, 2024
a5fbd5a
Release notes
joel-jeremy Sep 23, 2024
1bc77b5
Update packages/desktop-client/src/components/budget/BalanceWithCarry…
joel-jeremy Sep 25, 2024
f0aab63
Disable balance tooltip for mobile
joel-jeremy Sep 25, 2024
c89a594
Fix lint error
joel-jeremy Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
185 changes: 104 additions & 81 deletions packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import { useFeatureFlag } from '../../hooks/useFeatureFlag';
import { SvgArrowThinRight } from '../../icons/v1';
import { useResponsive } from '../../ResponsiveProvider';
import { type CSSProperties, theme, styles } from '../../style';
import { Tooltip } from '../common/Tooltip';
import { View } from '../common/View';
Expand Down Expand Up @@ -61,16 +62,25 @@
);
}

type CellValueChildren = ComponentPropsWithoutRef<typeof CellValue>['children'];

type ChildrenWithClassName = (
props: Parameters<CellValueChildren>[0] & {
className: string;
},
) => ReturnType<CellValueChildren>;

type BalanceWithCarryoverProps = Omit<
ComponentPropsWithoutRef<typeof CellValue>,
'binding'
'children' | 'binding'
> & {
children?: ChildrenWithClassName;
carryover: Binding<'envelope-budget', 'carryover'>;
balance: Binding<'envelope-budget', 'leftover'>;
goal: Binding<'envelope-budget', 'goal'>;
budgeted: Binding<'envelope-budget', 'budget'>;
longGoal: Binding<'envelope-budget', 'long-goal'>;
disabled?: boolean;
isDisabled?: boolean;
CarryoverIndicator?: ComponentType<CarryoverIndicatorProps>;
};

Expand All @@ -80,18 +90,19 @@
goal,
budgeted,
longGoal,
disabled,
isDisabled,
CarryoverIndicator: CarryoverIndicatorComponent = CarryoverIndicator,
children,
...props
}: BalanceWithCarryoverProps) {
const { t } = useTranslation();
const { isNarrowWidth } = useResponsive();
const carryoverValue = useSheetValue(carryover);
const goalValue = useSheetValue(goal);
const budgetedValue = useSheetValue(budgeted);
const longGoalValue = useSheetValue(longGoal);
const isGoalTemplatesEnabled = useFeatureFlag('goalTemplatesEnabled');
const getBalanceStyle = useCallback(
const getBalanceAmountStyle = useCallback(
(balanceValue: number) =>
makeBalanceAmountStyle(
balanceValue,
Expand All @@ -102,101 +113,113 @@
);
const format = useFormat();

const differenceToGoal = useCallback(
const getDifferenceToGoal = useCallback(
(balanceValue: number) =>
longGoalValue === 1
? balanceValue - goalValue
: budgetedValue - goalValue,
[budgetedValue, goalValue, longGoalValue],
);

const getDefaultClassName = useCallback(
(balanceValue: number) =>
String(
css({
...getBalanceAmountStyle(balanceValue),
overflow: 'hidden',
textOverflow: 'ellipsis',
textAlign: 'right',
...(!isDisabled && {
cursor: 'pointer',
}),
':hover': { textDecoration: 'underline' },
}),
),
[getBalanceAmountStyle, isDisabled],
Comment on lines +125 to +138
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove unnecessary String() wrapping around css().

The css() function already returns a string representing the class name. Wrapping it with String() is redundant.

Apply this diff to simplify the code:

-    (balanceValue: number) =>
-      String(
-        css({
-          ...getBalanceAmountStyle(balanceValue),
-          overflow: 'hidden',
-          textOverflow: 'ellipsis',
-          textAlign: 'right',
-          ...(!isDisabled && {
-            cursor: 'pointer',
-          }),
-          ':hover': { textDecoration: 'underline' },
-        }),
-      ),
+    (balanceValue: number) =>
+      css({
+        ...getBalanceAmountStyle(balanceValue),
+        overflow: 'hidden',
+        textOverflow: 'ellipsis',
+        textAlign: 'right',
+        ...(!isDisabled && {
+          cursor: 'pointer',
+        }),
+        ':hover': { textDecoration: 'underline' },
+      }),
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
(balanceValue: number) =>
String(
css({
...getBalanceAmountStyle(balanceValue),
overflow: 'hidden',
textOverflow: 'ellipsis',
textAlign: 'right',
...(!isDisabled && {
cursor: 'pointer',
}),
':hover': { textDecoration: 'underline' },
}),
),
[getBalanceAmountStyle, isDisabled],
(balanceValue: number) =>
css({
...getBalanceAmountStyle(balanceValue),
overflow: 'hidden',
textOverflow: 'ellipsis',
textAlign: 'right',
...(!isDisabled && {
cursor: 'pointer',
}),
':hover': { textDecoration: 'underline' },
}),
[getBalanceAmountStyle, isDisabled],

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

glamor's css function returns a StyleAttribute, not a string

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joel-jeremy, thank you for the clarification. You're absolutely right—glamor's css() function returns a StyleAttribute, not a string. My apologies for the oversight.

(·•·)?

✏️ Learnings added 
Learnt from: joel-jeremy
PR: actualbudget/actual#3492
File: packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx:123-136
Timestamp: 2024-09-25T21:00:54.812Z
Learning: glamor's `css()` function returns a `StyleAttribute`, not a string.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

);
Comment on lines +124 to +139
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove unnecessary String() wrapping around css().

The getDefaultClassName function looks good overall, but there's an unnecessary String() wrapping around the css() call. According to the learning note, css() returns a StyleAttribute, not a string. The String() wrapping is redundant and potentially incorrect.

Apply this diff to fix the issue:

   const getDefaultClassName = useCallback(
     (balanceValue: number) =>
-      String(
-        css({
-          ...getBalanceAmountStyle(balanceValue),
-          overflow: 'hidden',
-          textOverflow: 'ellipsis',
-          textAlign: 'right',
-          ...(!isDisabled && {
-            cursor: 'pointer',
-          }),
-          ':hover': { textDecoration: 'underline' },
-        }),
-      ),
+      css({
+        ...getBalanceAmountStyle(balanceValue),
+        overflow: 'hidden',
+        textOverflow: 'ellipsis',
+        textAlign: 'right',
+        ...(!isDisabled && {
+          cursor: 'pointer',
+        }),
+        ':hover': { textDecoration: 'underline' },
+      }),
     [getBalanceAmountStyle, isDisabled],
   );

This change ensures that the StyleAttribute returned by css() is used directly, which is the correct approach when working with glamor's css() function.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const getDefaultClassName = useCallback(
(balanceValue: number) =>
String(
css({
...getBalanceAmountStyle(balanceValue),
overflow: 'hidden',
textOverflow: 'ellipsis',
textAlign: 'right',
...(!isDisabled && {
cursor: 'pointer',
}),
':hover': { textDecoration: 'underline' },
}),
),
[getBalanceAmountStyle, isDisabled],
);
const getDefaultClassName = useCallback(
(balanceValue: number) =>
css({
...getBalanceAmountStyle(balanceValue),
overflow: 'hidden',
textOverflow: 'ellipsis',
textAlign: 'right',
...(!isDisabled && {
cursor: 'pointer',
}),
':hover': { textDecoration: 'underline' },
}),
[getBalanceAmountStyle, isDisabled],
);


return (
<CellValue binding={balance} type="financial" {...props}>
{({ type, name, value: balanceValue }) => (
<>
{children ? (
children({ type, name, value: balanceValue })
) : (
<Tooltip
content={
<View style={{ padding: 10 }}>
<span style={{ fontWeight: 'bold' }}>
{differenceToGoal(balanceValue) === 0 ? (
<span style={{ color: theme.noticeText }}>
{t('Fully funded')}
</span>
) : differenceToGoal(balanceValue) > 0 ? (
<span style={{ color: theme.noticeText }}>
{t('Overfunded ({{amount}})', {
amount: format(
differenceToGoal(balanceValue),
'financial',
),
})}
</span>
) : (
<span style={{ color: theme.errorText }}>
{t('Underfunded ({{amount}})', {
amount: format(
differenceToGoal(balanceValue),
'financial',
),
})}
</span>
)}
</span>
<GoalTooltipRow>
<div>{t('Goal Type:')}</div>
<div>{longGoalValue === 1 ? 'Long' : 'Template'}</div>
</GoalTooltipRow>
<GoalTooltipRow>
<div>{t('Goal:')}</div>
<div>{format(goalValue, 'financial')}</div>
</GoalTooltipRow>
<GoalTooltipRow>
{longGoalValue !== 1 ? (
<>
<div>{t('Budgeted:')}</div>
<div>{format(budgetedValue, 'financial')}</div>
</>
) : (
<>
<div>{t('Balance:')}</div>
<div>{format(balanceValue, type)}</div>
</>
)}
</GoalTooltipRow>
</View>
}
style={{ ...styles.tooltip, borderRadius: '0px 5px 5px 0px' }}
placement="bottom"
triggerProps={{
delay: 750,
isDisabled: !isGoalTemplatesEnabled || goalValue == null,
}}
>
<Tooltip
content={
<View style={{ padding: 10 }}>
<span style={{ fontWeight: 'bold' }}>
{getDifferenceToGoal(balanceValue) === 0 ? (
<span style={{ color: theme.noticeText }}>
{t('Fully funded')}
</span>
) : getDifferenceToGoal(balanceValue) > 0 ? (
<span style={{ color: theme.noticeText }}>
{t('Overfunded ({{amount}})', {
amount: format(
getDifferenceToGoal(balanceValue),
'financial',
),
})}
</span>
) : (
<span style={{ color: theme.errorText }}>
{t('Underfunded ({{amount}})', {
amount: format(
getDifferenceToGoal(balanceValue),
'financial',
),
})}
</span>
)}
</span>
<GoalTooltipRow>
<div>{t('Goal Type:')}</div>
<div>{longGoalValue === 1 ? t('Long') : t('Template')}</div>
</GoalTooltipRow>
<GoalTooltipRow>
<div>{t('Goal:')}</div>
<div>{format(goalValue, 'financial')}</div>
</GoalTooltipRow>
<GoalTooltipRow>
{longGoalValue !== 1 ? (
<>
<div>{t('Budgeted:')}</div>
<div>{format(budgetedValue, 'financial')}</div>
</>
) : (
<>
<div>{t('Balance:')}</div>
<div>{format(balanceValue, type)}</div>
</>
)}
</GoalTooltipRow>
</View>
}
style={{ ...styles.tooltip, borderRadius: '0px 5px 5px 0px' }}
placement="bottom"
triggerProps={{
delay: 750,
isDisabled: !isGoalTemplatesEnabled || goalValue == null || isNarrowWidth,

Check warning on line 200 in packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx

View workflow job for this annotation

GitHub Actions / lint 

Insert `⏎···············`
}}
>
{children ? (
children({
type,
name,
value: balanceValue,
className: getDefaultClassName(balanceValue),
})
) : (
<CellValueText
type={type}
name={name}
value={balanceValue}
className={String(
css({
...getBalanceStyle(balanceValue),
overflow: 'hidden',
textOverflow: 'ellipsis',
textAlign: 'right',
...(!disabled && {
cursor: 'pointer',
}),
':hover': { textDecoration: 'underline' },
}),
)}
className={getDefaultClassName(balanceValue)}
/>
</Tooltip>
)}
)}
</Tooltip>

{carryoverValue && (
<CarryoverIndicatorComponent
style={getBalanceStyle(balanceValue)}
style={getBalanceAmountStyle(balanceValue)}
/>
)}
</>
Expand Down
2 changes: 1 addition & 1 deletion packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@ export const CategoryMonth = memo(function CategoryMonth({
onClick={() => !category.is_income && setBalanceMenuOpen(true)}
>
<BalanceWithCarryover
disabled={category.is_income}
isDisabled={category.is_income}
carryover={trackingBudget.catCarryover(category.id)}
balance={trackingBudget.catBalance(category.id)}
goal={trackingBudget.catGoal(category.id)}
Expand Down
2 changes: 1 addition & 1 deletion packages/desktop-client/src/components/modals/EnvelopeBalanceMenuModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export function EnvelopeBalanceMenuModal({
Balance
</Text>
<BalanceWithCarryover
disabled
isDisabled
carryover={envelopeBudget.catCarryover(categoryId)}
balance={envelopeBudget.catBalance(categoryId)}
goal={envelopeBudget.catGoal(categoryId)}
Expand Down
2 changes: 1 addition & 1 deletion packages/desktop-client/src/components/modals/TrackingBalanceMenuModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export function TrackingBalanceMenuModal({
Balance
</Text>
<BalanceWithCarryover
disabled
isDisabled
carryover={trackingBudget.catCarryover(categoryId)}
balance={trackingBudget.catBalance(categoryId)}
goal={trackingBudget.catGoal(categoryId)}
Expand Down
6 changes: 6 additions & 0 deletions upcoming-release-notes/3492.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
category: Bugfix
authors: [joel-jeremy]
---

Fix mobile balance modal not properly coloring balance.
Loading