test: color-no-hex confirmations batch#27149
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
…useEmptyNavHeaderForConfirmations
| import styleSheet from './bottom-modal.styles'; | ||
|
|
||
| const OPAQUE_GRAY = '#414141'; | ||
| const OPAQUE_GRAY = brandColor.grey600; |
There was a problem hiding this comment.
We should avoid using colors outside of our design tokens. brandColor.grey600 (#4b505c) is the closest token to #414141, so we should use that instead of introducing a new hex value.
We also do not currently have a fully opaque overlay token. I am curious what hideBackground is intended for. From a quick look, hideBackground does not appear to be used. Should it be removed? We should likely be using BottomSheet instead of BottomModal anway?
@MetaMask/confirmations
backdropColor={hideBackground ? OPAQUE_GRAY : colors.overlay.default}
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection: Performance Test Selection: |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed:
brandColor.grey600may not exist, causing undefined backdrop- Replaced
brandColor.grey600with confirmed tokenbrandColor.grey900to ensure a defined backdrop color.
- Replaced
Or push these changes by commenting:
@cursor push d6400f12c4
Preview (d6400f12c4)
diff --git a/app/components/Views/confirmations/components/UI/bottom-modal/bottom-modal.tsx b/app/components/Views/confirmations/components/UI/bottom-modal/bottom-modal.tsx
--- a/app/components/Views/confirmations/components/UI/bottom-modal/bottom-modal.tsx
+++ b/app/components/Views/confirmations/components/UI/bottom-modal/bottom-modal.tsx
@@ -8,7 +8,7 @@
import { useStyles } from '../../../../../hooks/useStyles';
import styleSheet from './bottom-modal.styles';
-const OPAQUE_GRAY = brandColor.grey600;
+const OPAQUE_GRAY = brandColor.grey900;
interface BottomModalProps {
avoidKeyboard?: boolean;
children: ReactChild;| import styleSheet from './bottom-modal.styles'; | ||
|
|
||
| const OPAQUE_GRAY = '#414141'; | ||
| const OPAQUE_GRAY = brandColor.grey600; |
There was a problem hiding this comment.
brandColor.grey600 may not exist, causing undefined backdrop
High Severity
OPAQUE_GRAY was changed from '#414141' to brandColor.grey600, but grey600 may not be a valid property on the brandColor export from @metamask/design-tokens. Other usages of brandColor in the codebase (e.g., KYCFailed.tsx) only reference brandColor.white. If grey600 is undefined, the modal's backdropColor will be undefined when hideBackground is true, resulting in no visible backdrop — a production visual regression for the confirmation modal.
There was a problem hiding this comment.
It exists... chill
| headerTitleAlign: 'center' as const, | ||
| headerStyle: { | ||
| backgroundColor: '#ffffff', | ||
| backgroundColor: mockTheme.colors.background.default, |
There was a problem hiding this comment.
When '#ffffff' is hardcoded in a test assertion, any @metamask/design-tokens upgrade that changes background.default will cause this test to fail even though the component is rendering the correct token value. By referencing mockTheme.colors.background.default, the assertion always matches whatever the current token resolves to, making this test resilient to package upgrades.
| }, | ||
| }, | ||
| })), | ||
| theme: mockTheme, |
There was a problem hiding this comment.
The previous mock returned theme: { colors: { text: { alternative: '#6a737d' } } } — a partial, hardcoded copy of a design token value. If @metamask/design-tokens upgrades that color, the component renders the new value but the test mock still returns the old hex, producing a spurious failure. Replacing it with mockTheme (the full canonical test theme) means the mock always stays in sync with the token package automatically.
| import { InfoRowDivider } from './divider'; | ||
| import renderWithProvider from '../../../../../../../util/test/renderWithProvider'; | ||
| import { lightTheme } from '@metamask/design-tokens'; | ||
| import { mockTheme } from '../../../../../../../util/theme'; |
There was a problem hiding this comment.
Using lightTheme directly here achieves the same thing, we will not see a test failure if the design tokens are updated. mockTheme uses lightTheme and is the convention we are using to mock colors.
| const mockNft = createMockNft({ | ||
| standard: 'ERC721', | ||
| collectionName: 'Bored Apes', | ||
| // eslint-disable-next-line @metamask/design-tokens/color-no-hex |
There was a problem hiding this comment.
Strings like '#456', '#12345', '#22222' are NFT token IDs — # is a display prefix (e.g. #<tokenId>), not a CSS hex color. The color-no-hex rule can't distinguish between them, so these targeted suppressions are intentional. The rule is being enforced across the codebase to prevent real static hex values from creeping into tests (which would make them brittle on package upgrades), so the inline disables are the right way to allow these legitimate domain strings without weakening the broader rule.
|
✅ E2E Fixture Validation — Schema is up to date |
|
Description
Split from #26651 to reduce CODEOWNERS fanout.
Batch: confirmations
Source branch: `origin/remove-static-hex-from-tests`
When `@metamask/design-tokens` upgrades change color values, tests that hardcode hex literals (e.g. `'#ffffff'`) break because the component renders the new token value while the test still asserts the old one — even though nothing is actually wrong. This PR fixes that brittleness across the confirmations scope.
Two strategies are applied:
Replace hardcoded hex with token references — test mocks and assertions use `mockTheme.colors.` (or `brandColor.` for production code) so the test always agrees with whatever the current token value is. Package upgrades no longer cause spurious failures.
Add targeted `eslint-disable` comments — strings like `'chore: bump walletconnect/* deps #12345'` or `'ENS / IPFS resolution is broken #456'` are NFT token IDs that happen to look like hex colors. The `color-no-hex` rule can't distinguish them from CSS colors, so suppress comments are added inline to keep the rule enforced everywhere else without false positives.
Together these changes make the `@metamask/design-tokens/color-no-hex` lint rule enforceable across this scope, so future hex literals won't accidentally creep in.
Changelog
CHANGELOG entry: null
Related issues
Fixes:
Manual testing steps
```gherkin
Feature: color-no-hex lint compliance (confirmations batch)
Scenario: user runs lint
Given the confirmations test files have been updated
When user runs the lint checks
Then no color-no-hex violations are reported
```
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist