Update tooltip and themes with better visibility #3298
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
|
psybers
changed the title
|
@VoltaicGRiD I revived your old PR and updated VRT. |
|
The tooltips in light theme seem off. Like it has a shadow over the whole thing. The dark modes seem fine. |
|
I think I see what you mean. The background isn't white, like the table cells, it is somewhat off. |
|
|
@youngcw How about this? |
|
thats better |
There was a problem hiding this comment.
Does this stay as needing updated if you re-run the vrt?
There was a problem hiding this comment.
It does. Not sure why it hovers now when it did not before.
|
This adds borders around the payee and category pickers. It would be nice if those stayed the same. |
@youngcw Changed. None of the autocomplete menus show the border now. |
...actions.test.js-snapshots/Transactions-filters-transactions-by-category-4-chromium-linux.png
Outdated
Show resolved
Hide resolved
...ransactions.test.js-snapshots/Transactions-filters-transactions-by-date-4-chromium-linux.png
Outdated
Show resolved
Hide resolved
|
Im guessing that you are letting those filter tooltips be different on purpose. Thats fine, but the VRT needs updated. |
I'm not sure why those VRT fail. The new version has an extra filter button for tags, but the 'expected' does not. I did not touch any filters... |
WalkthroughThe changes introduce enhancements to the styling of various components, particularly focusing on tooltips and popovers across multiple theme files. The Changes
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: 1
Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/select/DateSelect.tsx (2)
338-338: ConsolidateminWidthintostyles.popoverfor consistencyTo maintain consistency and centralize styling, consider moving the
minWidth: 225property into thestyles.popoverobject within your styles module. This approach promotes reusability and simplifies maintenance.
Line range hint
217-235: Safely handlenullselectedValueinonBlurhandlerIn the
onBlurhandler, ifselectedValueisnull, callingparse(selectedValue, dateFormat, new Date())may result in an error. Ensure thatselectedValueis notnullbefore parsing to prevent potential runtime errors.Apply this diff to add a null check:
} else { setValue(selectedValue || ''); + if (selectedValue) { const date = parse(selectedValue, dateFormat, new Date()); if (date instanceof Date && !isNaN(date.valueOf())) { onSelect(format(date, 'yyyy-MM-dd')); } + } }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (11)
packages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-9-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-category-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-category-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-category-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-category-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-category-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-category-6-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-date-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-date-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-date-3-chromium-linux.pngis excluded by!**/*.pngupcoming-release-notes/3298.mdis excluded by!**/*.md
Files selected for processing (7)
- packages/desktop-client/src/components/autocomplete/Autocomplete.tsx (1 hunks)
- packages/desktop-client/src/components/select/DateSelect.tsx (2 hunks)
- packages/desktop-client/src/style/styles.ts (1 hunks)
- packages/desktop-client/src/style/themes/dark.ts (1 hunks)
- packages/desktop-client/src/style/themes/development.ts (1 hunks)
- packages/desktop-client/src/style/themes/light.ts (1 hunks)
- packages/desktop-client/src/style/themes/midnight.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/desktop-client/src/components/autocomplete/Autocomplete.tsx
Additional comments not posted (7)
packages/desktop-client/src/style/styles.ts (2)
142-143: Approved: Popover style border set to 'none'Setting the
borderproperty of thepopoverstyle to'none'improves visual consistency and aligns with the intended design.
134-141: Ensure theme variables for tooltip styling are definedThe added properties in the
tooltipstyle rely on theme variables (theme.tooltipBorder,theme.tooltipBackground,theme.tooltipText). Please verify that these theme variables are defined in all theme files to prevent runtime errors.Run the following script to confirm that the tooltip theme variables are defined:
Verification successful
Tooltip theme variables are properly defined across all themes
The verification process confirms that
tooltipBorder,tooltipBackground, andtooltipTextare correctly defined in all theme files (midnight, development, light, and dark). This ensures consistent tooltip styling across different visual modes and mitigates the risk of runtime errors.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that tooltip theme variables are defined in theme files. # Test: Search for definitions of 'tooltipBorder', 'tooltipBackground', and 'tooltipText'. # Expect: Occurrences of these variables in theme files. rg --type js --type ts --word-regexp 'tooltip(Border|Background|Text)'Length of output: 1559
packages/desktop-client/src/style/themes/development.ts (1)
209-211: Addition of tooltip styling constants follows established patterns.The newly added
tooltipText,tooltipBackground, andtooltipBorderconstants are appropriately named and utilize colors from the existingcolorPalette, maintaining consistency with the rest of the theme definitions.packages/desktop-client/src/style/themes/dark.ts (1)
210-212: LGTM!The addition of tooltip styling constants improves the customization and consistency of tooltips in the dark theme.
packages/desktop-client/src/style/themes/light.ts (1)
212-214: New tooltip styling constants added correctlyThe addition of
tooltipText,tooltipBackground, andtooltipBorderis consistent with the existing naming conventions and structure of the theme file. These constants will enhance the customization and visibility of tooltips in the light theme.packages/desktop-client/src/style/themes/midnight.ts (1)
212-214: Consistent addition of tooltip styling constantsThe new constants
tooltipText,tooltipBackground, andtooltipBorderare appropriately defined and align well with the existing midnight theme color palette. This addition enhances tooltip visibility and maintains visual consistency across the application.packages/desktop-client/src/components/select/DateSelect.tsx (1)
30-30: Appropriate import of 'styles' and 'theme'Importing
{ styles, theme }from'../../style'enhances consistency and reusability of styles across components.
| export const tooltipText = colorPalette.navy900; | ||
| export const tooltipBackground = colorPalette.white; | ||
| export const tooltipBorder = colorPalette.navy150; |
There was a problem hiding this comment.
Consider adding tooltip styling constants to other themes for consistency
To maintain a consistent user experience across all themes, consider adding the tooltipText, tooltipBackground, and tooltipBorder constants to the other theme files (e.g., dark.ts, midnight.ts). This ensures that tooltips are styled appropriately regardless of the selected theme.
There was a problem hiding this comment.
Swing and a miss... suggesting things that were already done.
There was a problem hiding this comment.
@psybers Thank you for pointing that out. I see that you've already added the tooltip styling constants to the other theme files. I'll make sure to be more attentive in future reviews.
🐇🤔
Learnings added
Learnt from: psybers
PR: actualbudget/actual#3298
File: packages/desktop-client/src/style/themes/light.ts:212-214
Timestamp: 2024-09-20T03:54:11.311Z
Learning: The tooltip styling constants `tooltipText`, `tooltipBackground`, and `tooltipBorder` have been added to all theme files, including `dark.ts`, `development.ts`, and `midnight.ts`.
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.
Reviving #2815.