Fix React Aria Button hover styles #3453
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. |
joel-jeremy
changed the title
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 No assets were bigger 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
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller
Unchanged No assets were unchanged |
There was a problem hiding this comment.
I've double checked as best I can but this doesn't seem to replicate the behaviour in demo (which is a little odd anyway IMO)
Just going off the report in #3447 I was testing the payee selector in the rules creation modal
On edge: as described in the bug
On demo: no hover effect, grayed out background on click that does not persist
In this change: hover effect changes text colour to black, but click effect isn't there
Can send videos if you can't reproduce but I've cleared cache etc so it should be the same for everyone
| @@ -139,6 +139,7 @@ export const Button = forwardRef<HTMLButtonElement, ButtonProps>( | |||
| children, | |||
| variant = 'normal', | |||
| bounce = true, | |||
| // style, | |||
There was a problem hiding this comment.
Was this left in on purpose?
|
@coderabbitai full review |
Actions performedFull review triggered. |
WalkthroughThe changes involve a comprehensive refactor of styling across multiple components in the desktop client, transitioning from inline styles to a CSS-in-JS approach using the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Recent review detailsConfiguration used: CodeRabbit UI Files ignored due to path filters (1)
Files selected for processing (16)
Files skipped from review as they are similar to previous changes (16)
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 (
|
If I remember correctly, the behavior in this PR was how it used to work before. If not, I think the hover effect made in this PR is a better experience than the one in edge/demo. |
I like that we have a hover effect, but I wonder, could we make it dim the background instead of setting the text colour (similar to other buttons when hovered)? Right now it's only visible on the payee selector in midnight mode, but not the op selector. I also think it would be worth persisting the effect on focus/when the select is open—I think this is how many other apps indicate a select. e.g. see native one in Safari which persists the focus effect: |
![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/settings/Themes.tsx (1)
Line range hint
1-100: Consider enhancing the hover effect based on the feedback.In the pull request comments, user jfdoming suggested improving the hover effect by dimming the background instead of changing the text color, aligning it with the hover effects of other buttons in the application.
They also recommended that the hover effect should persist on focus or when the select is open, similar to the behavior of native select elements in Safari.
To provide a more consistent and intuitive user experience, consider implementing the following enhancements:
- Modify the hover effect to dim the background color instead of changing the text color.
- Ensure that the hover effect persists when the select element is focused or open.
This will improve the visual feedback and make the hover behavior more aligned with user expectations and native select element behavior.
packages/desktop-client/src/components/modals/EditRuleModal.jsx (1)
141-141: The style prop is not being utilized effectively.The
styleprop is being passed to theOpSelectcomponent, but it is not being applied to any specific element within the component. This prop is not serving any purpose and can be removed to simplify the code.Remove the
styleprop from theOpSelectcomponent usage:- style={style}
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- packages/desktop-client/src/components/common/Button2.tsx (1 hunks)
- packages/desktop-client/src/components/common/Select.tsx (4 hunks)
- packages/desktop-client/src/components/modals/EditRuleModal.jsx (2 hunks)
- packages/desktop-client/src/components/modals/ImportTransactionsModal.jsx (1 hunks)
- packages/desktop-client/src/components/reports/Header.tsx (1 hunks)
- packages/desktop-client/src/components/select/RecurringSchedulePicker.tsx (3 hunks)
- packages/desktop-client/src/components/settings/Format.tsx (3 hunks)
- packages/desktop-client/src/components/settings/Themes.tsx (2 hunks)
Files skipped from review due to trivial changes (3)
- packages/desktop-client/src/components/modals/ImportTransactionsModal.jsx
- packages/desktop-client/src/components/select/RecurringSchedulePicker.tsx
- packages/desktop-client/src/components/settings/Format.tsx
Files skipped from review as they are similar to previous changes (2)
- packages/desktop-client/src/components/common/Button2.tsx
- packages/desktop-client/src/components/common/Select.tsx
Additional comments not posted (4)
packages/desktop-client/src/components/settings/Themes.tsx (2)
Line range hint
80-85: Verify the impact of the style change on the select's hover behavior.Similar to the previous comment, the change replaces the
:hoverpseudo-class with a custom attribute selector&[data-hovered]for styling the select's hover state.Please ensure that this change does not introduce any regression in the select's hover behavior and that the hover effect is applied consistently across different browsers and devices, especially considering the bug reported in the linked issue (#3447) related to the hover effect on Edge browser.
To verify the impact of this change, you can run the following script:
#!/bin/bash # Description: # Verify the usage and behavior of the `data-hovered` attribute in the Select component. # Test 1: Search for the Select component file. # Expect: Find the Select.tsx file. echo "Searching for the Select component file..." fd --type f --extension tsx Select # Test 2: Search for custom hover handling logic in the Select component. # Expect: Find code snippets that manually set the `data-hovered` attribute based on hover state. echo "Searching for custom hover handling logic in Select component..." rg --type tsx 'onMouseEnter|onMouseLeave' $(fd --type f --extension tsx Select)If the above tests yield relevant results, review the code to ensure that the
data-hoveredattribute is being set correctly based on the hover state and that there are no inconsistencies or potential issues in the hover handling logic specific to the Select component.Additionally, manually test the select's hover behavior across different browsers, particularly on Edge, to verify that the hover effect is applied consistently and resolves the flickering issue reported in the bug.
Compare the behavior with the demo site and ensure that the hover effect aligns with the expected behavior outlined in the linked issue.
65-69: Verify the impact of the style change on the button's hover behavior.The change replaces the
:hoverpseudo-class with a custom attribute selector&[data-hovered]for styling the button's hover state. This modification suggests a shift in how the hover state is managed, potentially using adata-hoveredattribute instead of relying on the CSS:hoverpseudo-class.Please ensure that this change does not introduce any regression in the button's hover behavior and that the hover effect is applied consistently across different browsers and devices.
To verify the impact of this change, you can run the following script:
If the above tests yield relevant results, review the code to ensure that the
data-hoveredattribute is being set correctly based on the hover state and that there are no inconsistencies or potential issues in the hover handling logic.Additionally, manually test the button's hover behavior across different browsers and devices to verify that the hover effect is applied consistently and as expected.
Verification successful
Verified: The style change is consistent with the project's component styling approach.
The change from
:hoverto&[data-hovered]in Themes.tsx is part of a broader pattern in the project for certain components, particularly Select components. This approach allows for more controlled hover states and is consistently applied within the file and across other components.Key observations:
- The
[data-hovered]attribute is used in multiple files, including Themes.tsx.- Both Select components in Themes.tsx use the same
[data-hovered]styling approach.- The
:hoverpseudo-class is still used elsewhere in the project, indicating this is a targeted change rather than a global replacement.This change is unlikely to introduce any regression in hover behavior and appears to be a deliberate styling decision for specific components.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: # Verify the usage and behavior of the `data-hovered` attribute in the codebase. # Test 1: Search for the usage of `data-hovered` attribute. # Expect: Find relevant code snippets that set or modify this attribute. echo "Searching for 'data-hovered' attribute usage..." rg --type tsx 'data-hovered' # Test 2: Search for the component file that defines the hover behavior. # Expect: Find the relevant component file (e.g., Button.tsx). echo "Searching for the component file defining hover behavior..." fd --type f --extension tsx Button # Test 3: Search for any custom hover handling logic in the component. # Expect: Find code snippets that manually set the `data-hovered` attribute based on hover state. echo "Searching for custom hover handling logic..." rg --type tsx 'onMouseEnter|onMouseLeave'Length of output: 1373
Script:
#!/bin/bash # Search for '[data-hovered]' attribute usage echo "Searching for '[data-hovered]' attribute usage..." rg "\[data-hovered\]" # Search for ':hover' pseudo-class usage echo "Searching for ':hover' pseudo-class usage..." rg ":hover" # Search for custom hover handling logic echo "Searching for custom hover handling logic..." rg "onMouseEnter|onMouseLeave" # Check the specific file where the change was made echo "Checking Themes.tsx file..." cat packages/desktop-client/src/components/settings/Themes.tsxLength of output: 12904
packages/desktop-client/src/components/reports/Header.tsx (2)
Line range hint
1-136: LGTM!The rest of the code changes in this file look good. The modifications are minimal and do not introduce any apparent issues or inconsistencies. The overall structure and logic of the component remain intact.
Also applies to: 138-210
137-137: Verify the impact of the style property change.The change from
buttonStyletostylesuggests a shift in the styling approach. Confirm that this change is intentional and aligns with the overall styling strategy in the codebase. Ensure that the newstyleproperty is correctly handled by theSelectcomponent and does not introduce any unintended visual or behavioral changes.To verify the impact of this change, run the following script:
Verification successful
Change to
styleprop is correct and consistentThe change from
buttonStyletostylein theSelectcomponent is verified to be correct and consistent with the codebase. TheSelectcomponent properly handles thestyleprop, applying it to its internal Button component. This change aligns with the overall styling strategy used across different instances of theSelectcomponent in the project.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: # - Verify the usage of the `style` prop in the `Select` component. # - Check if the `Select` component correctly handles the `style` prop. # Test 1: Search for the `Select` component usage and analyze the prop usage. # Expect: Consistent usage of the `style` prop across instances. rg --type tsx -A 5 $'<Select\b' # Test 2: Verify the implementation of the `Select` component. # Expect: The component should correctly handle the `style` prop and apply the styles. ast-grep --lang tsx --pattern $'export function Select($$$) { $$$ return <$_ $$$ style={$_} $$$ /> $$$ }'Length of output: 186
Script:
#!/bin/bash # Description: # - Verify the usage of the `style` prop in the `Select` component. # - Check if the `Select` component is defined in the codebase or imported. # Test 1: Search for the `Select` component usage in .ts and .tsx files rg --type-add 'typescript:*.{ts,tsx}' --type typescript -A 5 '<Select\b' # Test 2: Look for any Select component definition ast-grep --lang typescript 'function Select($$$) { $$$ }' # Test 3: Check for Select component imports rg --type-add 'typescript:*.{ts,tsx}' --type typescript 'import.*Select.*from'Length of output: 18122
Script:
#!/bin/bash # Description: # - Examine the Select component implementation # - Verify style prop handling in the Select component # Test 1: View the Select component implementation cat packages/desktop-client/src/components/common/Select.tsx # Test 2: Check for style prop usage in the Select component rg --type-add 'typescript:*.{ts,tsx}' --type typescript 'style[=:]' packages/desktop-client/src/components/common/Select.tsx # Test 3: Verify consistent style prop usage across Select components rg --type-add 'typescript:*.{ts,tsx}' --type typescript '<Select\b.*style=' -A 2Length of output: 4091
Script:
#!/bin/bash # Description: # - Verify usage of style prop and other styling props for Select component # - Examine specific files for Select component usage and styling # Test 1: Check for any prop used for styling the Select component rg --type-add 'typescript:*.{ts,tsx}' --type typescript '<Select\b.*(?:style|className)=' -A 2 # Test 2: Examine specific files for Select component usage and styling cat packages/desktop-client/src/components/reports/Header.tsx cat packages/desktop-client/src/components/select/RecurringSchedulePicker.tsx # Test 3: Check for any custom styling applied to Select component rg --type-add 'typescript:*.{ts,tsx}' --type typescript '<Select.*\{.*\}' -A 2Length of output: 24016
| style={{ | ||
| color: theme.pageTextPositive, | ||
| '&[data-hovered]': { color: theme.pageTextPositive }, | ||
| }} |
There was a problem hiding this comment.
Refactor the inline style to a CSS-in-JS object.
To improve maintainability and readability, consider extracting the inline style object into a separate CSS-in-JS object using the glamor library. This will centralize the style definitions and make the JSX code cleaner.
- style={{
- color: theme.pageTextPositive,
- '&[data-hovered]': { color: theme.pageTextPositive },
- }}
+ style={styles.fieldSelect}Then, define the fieldSelect style object using glamor:
const styles = {
fieldSelect: css({
color: theme.pageTextPositive,
'&[data-hovered]': { color: theme.pageTextPositive },
}),
// ...other styles
};
joel-jeremy
force-pushed
the
react-aria-button-hover-styles
branch
from
b562e5a
to
b973147
Compare
|
This caused a regression where some css classes are not prioritized correctly: It might surface itself in other places when overriding styles. |
* Fiox hover styles and use className instead of inline to prepare for future css migration * Release notes * Cleanup * Update edit rule hover style
* marked files for translation * added release note * fixed linting warnings * 🐛 fix account filters being overridden (#3441) *Reduce package size (#3443) * reduce package size of all packages * release notes * Update beforePackHook.ts * [Maintenance] Cleanup react aria packages and dedupe (#3450) * Cleanup react aria packages and dedupe * Release notes * ♻️ (synced-prefs) moving the prefs from metadata.json to the db (#3423) *
Resolves #3447
Preferring to use className instead of the inline style syntax to allow for easier CSS library migration in the future