fix: remove hover effect in sign out button #5435
Conversation
WalkthroughThe changes ensure that the hover effect for disabled buttons is correctly applied, making the background color consistent and overriding any other styles. Additionally, the hover effect for the Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- theme/recipes/button-recipe.ts (3 hunks)
Additional Context Used
Biome (1)
theme/recipes/button-recipe.ts (1)
3-3: All these imports are only used as types.
Additional comments not posted (2)
theme/recipes/button-recipe.ts (2)
54-54: Ensure the use of!importantis absolutely necessary for overriding styles.While the use of
!importantcan solve specificity issues, it can also make future CSS maintenance harder. Verify if there's an alternative approach that could be used, such as increasing the specificity of the selector in a less forceful manner.
77-77: Consistency in handling hover states for disabled buttons.The changes consistently apply the
!importantrule across different button variants to ensure that the hover state is disabled. This aligns with the PR's objective to improve UI consistency by preventing hover effects on disabled buttons.Also applies to: 99-99
There was a problem hiding this comment.
Thanks for the PR @jeeanribeiro—I know there was some discussion before on the sign out button styles.
Investigating further, I think the best fix here is a one line deletion: the sign out button should never have a hover state in the first place.
<Button
color="lightModeInk.1"
- _hover={{ background: 'black' }}
opacity={!canSignOut ? 0.8 : undefined}
data-testid={SettingsSelectors.BtnSignOutActuallyDeleteWallet}
flexGrow={1}
disabled={!canSignOut}
onClick={() => canSignOut && onUserDeleteWallet()}
type="submit"
>
Sign out
</Button>Discussed this with the team, we'd prefer to avoid !important here. The problem is gone with this line, and there is a possibility in future that we might need to override this style.
jeeanribeiro
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
src/app/features/settings/sign-out/sign-out.tsx (1)
Line range hint
7-8: Consider removing unused named imports if they are only used as types.- import { Flex, HStack, styled } from 'leather-styles/jsx'; + import type { Flex, HStack, styled } from 'leather-styles/jsx';
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/app/features/settings/sign-out/sign-out.tsx (1 hunks)
Additional Context Used
Biome (1)
src/app/features/settings/sign-out/sign-out.tsx (1)
7-8: Some named imports are only used as types.
Additional comments not posted (3)
src/app/features/settings/sign-out/sign-out.tsx (3)
Line range hint
11-18: The form setup and submission logic are correctly implemented.
Line range hint
11-18: The use of theCalloutcomponent to provide wallet-specific warnings is well-implemented.
Line range hint
11-18: Ensure that the CSS changes for button hover effects align with the button properties set here.Verification successful
The previous script failed due to an unrecognized file type. Let's correct the file type and re-run the script to search for the CSS changes related to hover effects.
The previous script did not produce any output, indicating that the specific CSS change might not be present in the file we searched. To ensure thorough verification, we should search for any CSS changes related to hover effects in the entire repository.
The previous attempts to locate CSS changes related to hover effects did not yield any results. This suggests that either the hover effects are not defined in CSS files or they are defined in a different manner. To ensure thorough verification, we should search for hover effects in all relevant files, including TypeScript and JavaScript files, as they might contain inline styles or styled-components.
The search results indicate that there are multiple instances of hover effects defined across various files in the repository, including
theme/recipes/button-recipe.tsand other component files. This confirms that hover effects are indeed being managed within the codebase.Given this context, it appears that the hover effects are being handled appropriately and consistently across the repository. The changes in the
SignOutDialogcomponent should align with these hover effect definitions.
theme/recipes/button-recipe.tscontains several hover effect definitions, such as_hover: { bg: 'ink.background-secondary' }and_hover: { bg: 'unset' }.- Other files like
src/app/ui/pressable/pressable.tsxandsrc/app/ui/pages/welcome.layout.tsxalso define hover effects.Based on this information, the hover effects are being managed consistently, and the changes in the
SignOutDialogcomponent should not introduce any inconsistencies.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the CSS changes for hover effects are consistent with the button properties in this file. # Test: Search for the CSS changes in the mentioned file and ensure consistency. rg --type typescript --files-with-matches 'ink.background-secondary !important' theme/recipes/button-recipe.tsLength of output: 147
Script:
#!/bin/bash # Description: Verify that the CSS changes for hover effects are consistent with the button properties in this file. # Test: Search for the CSS changes in the mentioned file and ensure consistency. rg --type ts --files-with-matches 'ink.background-secondary !important' theme/recipes/button-recipe.tsLength of output: 104
Script:
#!/bin/bash # Description: Verify that the CSS changes for hover effects are consistent with the button properties in this file. # Test: Search for any CSS changes related to hover effects in the entire repository. rg --type css 'hover'Length of output: 21
Script:
#!/bin/bash # Description: Verify that the CSS changes for hover effects are consistent with the button properties in this file. # Test: Search for any hover effects in the entire repository, including TypeScript and JavaScript files. rg 'hover' --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}'Length of output: 12878
Hello @kyranjamie 👋 Thank you for the review! I’ve updated the PR to remove the line you mentioned. I also generally avoid using I appreciate how welcoming the team is with my PRs. While I'm currently addressing small UI issues, I'm eager to participate in broader UI discussions. Thanks again to the team for being so welcoming! 😄 |
|
Thanks again @jeeanribeiro for your contributions. I've merged this work here #5460 |
closes #5406
Resolving the issue only on the Sign out button does not prevent the problem from recurring in other instances. Therefore, I have set a higher specificity in the button's recipe for the disabled and hover state background attribute. This ensures that disabled buttons never have a different background in the hover state, addressing concerns raised in previous PRs.
While I'm generally not a proponent of using
!important, it is justified in this context to achieve the desired consistency across the application.Summary by CodeRabbit