[WIP] Add info text to Upcoming Length control #3639
Conversation
actual-github-bot
bot
changed the title
✅ 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
|
SamBobBarnes
changed the title
|
I put |
SamBobBarnes
changed the title
WalkthroughA new React component named Additionally, the Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/settings/Upcoming.tsx (3)
48-61: LGTM: Improved layout and added InfoCircle component.The changes to the
UpcomingLengthSettingscomponent improve the layout by aligning elements horizontally and adding theInfoCirclecomponent as requested. This aligns well with the PR objectives.Consider adding an
aria-labelto theInfoCirclecomponent for better accessibility. For example:- <InfoCircle /> + <InfoCircle aria-label="Information about upcoming length" />
99-127: LGTM with suggestions: InfoCircle visible state implementation.The visible state of the
InfoCirclecomponent is well-implemented with appropriate styling and the required informational text. However, there are a few suggestions for improvement:
- The tooltip positioning might cause issues on smaller screens. Consider using a more responsive approach.
- The tooltip text mentions a temporary limitation. As per the PR objectives, we should plan to remove this once the feature is updated.
Consider the following improvements:
- Use a positioning library or CSS to ensure the tooltip is always visible on screen.
- Add a TODO comment to remind about removing this temporary info text in the future:
<Text> + {/* TODO: Remove this info text once the feature is updated to show all instances of recurring transactions */} <strong>Only</strong> the first instance of a recurring transaction will be shown. </Text>
128-136: LGTM with suggestion: InfoCircle non-visible state implementation.The non-visible state of the
InfoCirclecomponent is well-implemented with appropriate event handlers. The use of a timeout in theonMouseOverhandler is a good approach to prevent accidental triggers.Consider adding an
onMouseLeavehandler to cancel the timeout if the user moves the mouse away quickly:<SvgInformationCircle style={{ height: '15px', cursor: 'pointer' }} onClick={() => setVisible(true)} - onMouseOver={() => setTimeout(() => setVisible(true), 500)} + onMouseOver={() => { + const timeoutId = setTimeout(() => setVisible(true), 500); + return () => clearTimeout(timeoutId); + }} + onMouseLeave={() => setVisible(false)} />This change will improve the user experience by preventing the tooltip from appearing if the user quickly moves the mouse away from the icon.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3639.mdis excluded by!**/*.md
📒 Files selected for processing (1)
- packages/desktop-client/src/components/settings/Upcoming.tsx (3 hunks)
🧰 Additional context used
🔇 Additional comments (3)
packages/desktop-client/src/components/settings/Upcoming.tsx (3)
10-10: LGTM: Import statement for SvgInformationCircle added correctly.The new import statement for
SvgInformationCircleis correctly added and is necessary for the newInfoCirclecomponent.
95-97: LGTM: InfoCircle component declaration.The
InfoCirclecomponent is correctly declared with proper state management and initialization based on the location hash. This approach supports deep linking to the info tooltip.
Line range hint
1-137: Overall LGTM with considerations for future updates.The implementation successfully adds the requested info text to the Upcoming Length control, aligning with the PR objectives. The code is well-structured and achieves the desired functionality.
Considerations for future updates:
- As mentioned in the PR objectives, this info text is temporary. Ensure there's a plan to remove it once the feature is updated to show all instances of recurring transactions.
- Regarding the release notes categorization, given that this is a temporary informational addition requested by a user, it would be more appropriate to categorize it as an "enhancement" rather than "maintenance". This change directly improves the user experience by providing additional context for the feature's current limitations.
To ensure consistency with the PR objectives, run the following script to check for any TODO comments related to this temporary change:
This will help track the temporary nature of this change and remind developers to update it in the future.
|
👋 Would you mind moving this feature under a feature flag? There's definitely more polish that we'd need to do before releasing it as a first party feature. |
Are you meaning just polish around the styling? Is there a component that already has reactive styling that I could use for the info box? The functionality is only the help text to clarify the feature added in PR #3310. This was asked for in the comment, but the pr was merged before I had a chance to add it. |
|
The entire functionality added in #3310 would need more polish. So ideally we put it under a feature flag until it's polished and ready to go as a first party feature. Would that be something you could do? |
|
Yes, I could do that. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/settings/Upcoming.tsx (3)
53-66: LGTM: UI improvements enhance user experienceThe updated layout and addition of the InfoCircle component improve the user interface and provide a way to display additional information, aligning with the PR objectives.
Consider adding an aria-label to the InfoCircle component for improved accessibility. For example:
- <InfoCircle /> + <InfoCircle aria-label="Additional information about upcoming length" />
100-142: InfoCircle component implementation looks good, with room for improvementThe InfoCircle component successfully implements the required functionality to display additional information about the upcoming length setting. However, there are a few areas where it could be improved:
Tooltip positioning: The current implementation uses hardcoded values for positioning, which might cause issues on different screen sizes or layouts. Consider using a positioning library or implementing a more dynamic positioning strategy.
Accessibility: The component could benefit from improved accessibility features.
Consider the following improvements:
- Use a positioning library or implement dynamic positioning:
import { usePopper } from 'react-popper'; // Inside the InfoCircle component const [referenceElement, setReferenceElement] = useState(null); const [popperElement, setPopperElement] = useState(null); const { styles, attributes } = usePopper(referenceElement, popperElement, { placement: 'top', }); // Update the SvgInformationCircle and tooltip View components <SvgInformationCircle ref={setReferenceElement} ... /> <View ref={setPopperElement} style={styles.popper} {...attributes.popper}> ... </View>
- Enhance accessibility:
- <SvgInformationCircle + <SvgInformationCircle + role="button" + aria-label="Additional information about upcoming length" + aria-expanded={visible} style={{ height: '15px', cursor: 'pointer' }} onClick={() => setVisible(false)} onMouseLeave={() => setVisible(false)} /> <View + role="tooltip" + aria-hidden={!visible} style={{ // ... existing styles }} >These changes will improve the component's flexibility and accessibility.
Line range hint
1-142: Overall implementation aligns with PR objectivesThe changes successfully implement the requested informational text for the Upcoming Length control and address the concerns raised in the PR comments by placing the feature under a feature flag. This allows for easy enabling/disabling of the feature as needed.
To clarify the temporary nature of this implementation, consider adding a TODO comment at the top of the file or near the UpcomingLengthSettings component:
// TODO: This implementation is temporary and should be removed once the // Upcoming Length feature is updated to show all instances of scheduled // transactions instead of only the first one.This will help future developers understand the context and temporary nature of these changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3639.mdis excluded by!**/*.md
📒 Files selected for processing (3)
- packages/desktop-client/src/components/settings/Upcoming.tsx (3 hunks)
- packages/desktop-client/src/hooks/useFeatureFlag.ts (1 hunks)
- packages/loot-core/src/types/prefs.d.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
packages/desktop-client/src/hooks/useFeatureFlag.ts (1)
11-11: New feature flag added correctly.The new feature flag
upcomingLengthAdjustmenthas been added correctly to theDEFAULT_FEATURE_FLAG_STATEobject. This addition aligns with the PR objectives and follows the existing pattern in the file. The flag is set tofalseby default, which is appropriate for a new feature that's still under development.A few points to consider:
- The flag name
upcomingLengthAdjustmentaccurately represents its purpose based on the PR description.- The addition doesn't introduce any side effects as it doesn't modify the existing logic of the
useFeatureFlagfunction.- Remember to update the
FeatureFlagtype inloot-core/src/types/prefs.d.tsto include this new flag.To ensure the
FeatureFlagtype has been updated, please run the following script:✅ Verification successful
Feature flag
upcomingLengthAdjustmenthas been successfully added.The
upcomingLengthAdjustmentflag has been correctly included in both theDEFAULT_FEATURE_FLAG_STATEobject withinpackages/desktop-client/src/hooks/useFeatureFlag.tsand theFeatureFlagtype inloot-core/src/types/prefs.d.ts. This addition aligns with the PR objectives and follows the existing patterns in the codebase. The flag is set tofalseby default, which is appropriate for a new feature under development.No issues were found during the verification process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the FeatureFlag type includes the new upcomingLengthAdjustment flag # Test: Search for the FeatureFlag type definition and check if it includes the new flag rg --type typescript 'type FeatureFlag =' -A 10 'loot-core/src/types/prefs.d.ts'Length of output: 115
Script:
#!/bin/bash # Description: Verify that the FeatureFlag type includes the new upcomingLengthAdjustment flag # Test: Search for the upcomingLengthAdjustment flag within the FeatureFlag type definition grep 'upcomingLengthAdjustment' 'loot-core/src/types/prefs.d.ts'Length of output: 126
Script:
#!/bin/bash # Description: Locate the FeatureFlag type definition and verify the inclusion of the upcomingLengthAdjustment flag # Step 1: Find the file(s) containing the FeatureFlag type definition grep -r 'type FeatureFlag' . # Step 2: Once the file is identified, search for the upcomingLengthAdjustment flag within it # Note: Replace <found_file_path> with the actual path from Step 1's output grep 'upcomingLengthAdjustment' <found_file_path>Length of output: 291
Script:
#!/bin/bash # Description: Locate the FeatureFlag type definition and verify the inclusion of the upcomingLengthAdjustment flag # Step 1: Find the file(s) containing the FeatureFlag type definition feature_flag_files=$(grep -rl 'type FeatureFlag' .) # Step 2: Search for the upcomingLengthAdjustment flag within the found file(s) for file in $feature_flag_files; do echo "Checking in $file:" grep 'upcomingLengthAdjustment' "$file" doneLength of output: 354
packages/desktop-client/src/components/settings/Upcoming.tsx (2)
9-11: LGTM: New imports added correctlyThe new imports for
useFeatureFlagandSvgInformationCircleare correctly added and necessary for the implemented changes.
42-43: LGTM: Feature flag implemented correctlyThe
upcomingLengthAdjustmentfeature flag is correctly implemented, allowing for conditional rendering of the component. This aligns with the PR objectives and provides a way to control the visibility of this feature.Also applies to: 47-48
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/settings/Upcoming.tsx (2)
48-51: LGTM: Improved layout structure.The new View component within the Column improves the layout by aligning items horizontally. The use of flexbox with gap property is a modern and efficient approach to spacing elements.
Consider extracting the inline styles to a separate constant or using a styled component for better maintainability. For example:
const rowStyle: CSSProperties = { flexDirection: 'row', alignItems: 'center', gap: 20, }; // Then in JSX <View style={rowStyle}>
Line range hint
1-100: Summary: Changes align with PR objectives, feature flag implementation needed.The modifications to the
UpcomingLengthSettingscomponent successfully add the requested info text and improve the layout structure. The code changes are well-implemented and align with the PR objectives.However, the main action item is to implement the feature flag as discussed in the PR comments. This is crucial for controlling the visibility of this feature until it's fully refined and ready for release.
To implement the feature flag:
- Update the
FeatureFlagtype inprefs.d.ts.- Add the
upcomingLengthAdjustmentflag toDEFAULT_FEATURE_FLAG_STATEinuseFeatureFlag.ts.- Use the
useFeatureFlaghook in this component to conditionally render theUpcomingLengthSettingsbased on the flag's state.Once these changes are made, the PR will be ready for final review and merge.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3639.mdis excluded by!**/*.md
📒 Files selected for processing (2)
- packages/desktop-client/src/components/common/InfoBubble.tsx (1 hunks)
- packages/desktop-client/src/components/settings/Upcoming.tsx (2 hunks)
🧰 Additional context used
🔇 Additional comments (4)
packages/desktop-client/src/components/common/InfoBubble.tsx (2)
1-11: LGTM: Imports and type definition are well-structured.The imports are comprehensive and the
InfoBubblePropstype is clearly defined. This provides a good foundation for the component.
1-52: Summary of InfoBubble component reviewThe InfoBubble component is a well-structured and functional addition to the project. It provides a useful way to display additional information to users. However, there are several areas where it could be improved:
- Add cleanup for the URL hash to prevent unexpected behavior during navigation.
- Refactor the styling approach for better maintainability and flexibility.
- Improve the hover behavior and ensure consistency between click and hover events.
Implementing these suggestions will enhance the component's robustness, maintainability, and user experience. Overall, this is a good addition to the project with room for refinement.
packages/desktop-client/src/components/settings/Upcoming.tsx (2)
12-12: LGTM: InfoBubble import added correctly.The import statement for the InfoBubble component is correctly placed and aligns with the PR objective of adding informational text to the Upcoming Length control.
64-64: LGTM: Proper closing of View component.The closing tag for the View component is correct and maintains the proper structure.
| export function InfoBubble({ label }: InfoBubbleProps) { | ||
| const location = useLocation(); | ||
| const [visible, setVisible] = useState(location.hash === '#info'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding cleanup for the URL hash.
The initialization of the visible state based on the URL hash is a good approach. However, consider adding a cleanup effect to remove the hash from the URL after initialization. This will prevent unexpected behavior if the user navigates away and back to the page.
Here's a suggested implementation:
import React, { useState, useEffect } from 'react';
import { useLocation, useNavigate } from 'react-router-dom';
// ... other imports ...
export function InfoBubble({ label }: InfoBubbleProps) {
const location = useLocation();
const navigate = useNavigate();
const [visible, setVisible] = useState(location.hash === '#info');
useEffect(() => {
if (location.hash === '#info') {
navigate(location.pathname + location.search, { replace: true });
}
}, [location, navigate]);
// ... rest of the component ...
}This change ensures that the '#info' hash is removed from the URL after the component mounts, providing a cleaner URL without affecting the initial visibility state.
| return visible ? ( | ||
| <View style={{ userSelect: 'none' }}> | ||
| <SvgInformationCircle | ||
| style={{ height: '15px', cursor: 'pointer' }} | ||
| onClick={() => setVisible(false)} | ||
| onMouseLeave={() => setVisible(false)} | ||
| /> | ||
| <View | ||
| style={{ | ||
| position: 'absolute', | ||
| left: '20px', | ||
| top: '-20px', | ||
| color: 'white', | ||
| backgroundColor: 'black', | ||
| borderRadius: '5px', | ||
| borderStyle: 'solid', | ||
| borderWidth: '1px', | ||
| borderColor: 'white', | ||
| padding: '5px', | ||
| width: '200px', | ||
| zIndex: 300, | ||
| }} | ||
| > | ||
| <Text>{label}</Text> | ||
| </View> | ||
| </View> | ||
| ) : ( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor styling for better maintainability and flexibility.
While the styling achieves the desired look, there are a few areas that could be improved:
- Consider moving inline styles to a separate CSS file or using a CSS-in-JS solution for better maintainability.
- The use of absolute positioning might cause layout issues in different contexts. Consider using a more flexible positioning approach.
- The z-index of 300 seems arbitrary. It's better to use a z-index scale defined at the application level to prevent conflicts.
Here's a suggested approach using CSS modules (assuming your project supports them):
- Create a file named
InfoBubble.module.css:
.container {
user-select: none;
position: relative;
}
.icon {
height: 15px;
cursor: pointer;
}
.bubble {
position: absolute;
left: 20px;
top: -20px;
color: white;
background-color: black;
border-radius: 5px;
border: 1px solid white;
padding: 5px;
width: 200px;
z-index: var(--z-index-tooltip, 10);
}- Update the component:
import styles from './InfoBubble.module.css';
// ... other imports ...
export function InfoBubble({ label }: InfoBubbleProps) {
// ... state initialization ...
return visible ? (
<View className={styles.container}>
<SvgInformationCircle
className={styles.icon}
onClick={() => setVisible(false)}
onMouseLeave={() => setVisible(false)}
/>
<View className={styles.bubble}>
<Text>{label}</Text>
</View>
</View>
) : (
// ... hidden state rendering ...
);
}This approach separates concerns, improves maintainability, and allows for more flexible positioning. The --z-index-tooltip CSS variable can be defined at the application level for consistent z-index management.
| <View style={{ userSelect: 'none' }}> | ||
| <SvgInformationCircle | ||
| style={{ height: '15px', cursor: 'pointer' }} | ||
| onClick={() => setVisible(true)} | ||
| onMouseOver={() => setTimeout(() => setVisible(true), 500)} | ||
| /> | ||
| </View> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve hover behavior and event consistency.
The current implementation has a few potential issues:
- The setTimeout for the mouse over event can't be cancelled, potentially leading to unexpected behavior if the user moves the mouse away quickly.
- There's an inconsistency between the immediate response to clicks and the delayed response to hovers.
Consider the following improvements:
import React, { useState, useRef, useCallback } from 'react';
// ... other imports ...
export function InfoBubble({ label }: InfoBubbleProps) {
const [visible, setVisible] = useState(false);
const timeoutRef = useRef<NodeJS.Timeout | null>(null);
const showBubble = useCallback(() => {
if (timeoutRef.current) clearTimeout(timeoutRef.current);
timeoutRef.current = setTimeout(() => setVisible(true), 500);
}, []);
const hideBubble = useCallback(() => {
if (timeoutRef.current) clearTimeout(timeoutRef.current);
setVisible(false);
}, []);
return (
<View className={styles.container}>
<SvgInformationCircle
className={styles.icon}
onClick={showBubble}
onMouseEnter={showBubble}
onMouseLeave={hideBubble}
/>
{visible && (
<View className={styles.bubble}>
<Text>{label}</Text>
</View>
)}
</View>
);
}This implementation:
- Uses a ref to store the timeout, allowing it to be cancelled.
- Applies the same delay to both click and hover events for consistency.
- Clears the timeout when the mouse leaves, preventing unexpected shows.
- Uses the useCallback hook to memoize the event handlers, optimizing performance.
These changes will make the component's behavior more predictable and consistent.
| <View title="Upcoming Length"> | ||
| <Select | ||
| options={options.map(x => [x.value || '7', x.label])} | ||
| value={upcomingLength} | ||
| onChange={newValue => setUpcomingLength(newValue)} | ||
| style={selectButtonStyle} | ||
| /> | ||
| </View> | ||
| <InfoBubble | ||
| label="Only the first instance of a recurring transaction | ||
| will be shown." | ||
| /> |
There was a problem hiding this comment.
Approved: InfoBubble added, but feature flag needed.
The Select component is now properly structured, and the InfoBubble component has been added with the correct informational text as requested. This aligns well with the PR objectives.
However, based on the discussion in the PR comments, this feature should be placed under a feature flag until it's fully refined.
Please implement a feature flag for the Upcoming Length adjustment feature. You can use the useFeatureFlag hook to conditionally render this component. For example:
import { useFeatureFlag } from '../../hooks/useFeatureFlag';
// Inside the component
const isUpcomingLengthAdjustmentEnabled = useFeatureFlag('upcomingLengthAdjustment');
// In the JSX
{isUpcomingLengthAdjustmentEnabled && (
// Wrap the entire UpcomingLengthSettings content here
)}Don't forget to update the FeatureFlag type and DEFAULT_FEATURE_FLAG_STATE as mentioned in the AI summary.
SamBobBarnes
force-pushed
the
Upcoming-length-help-text
branch
from
2ae3374
to
5338043
Compare
SamBobBarnes
changed the title
|
Feature flag has been moved to PR #3651 |
This info text was requested by @youngcw in PR #3310.
This should be removed when the feature is updated to no longer have the limitation of only displaying the first instance of a scheduled transaction.