Skip to content

fix: dsr avatar linting violations #674

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
May 22, 2025
Merged

fix: dsr avatar linting violations #674

merged 3 commits into from
May 22, 2025

Conversation

georgewrmarshall
Copy link
Contributor

Description

This PR improves the Avatar components in the design-system-react package by removing ESLint ignores and fixing TypeScript type issues.

  1. What is the reason for the change?

    • Several Avatar components were being ignored by ESLint checks
    • Components were using type assertions (as any) which can hide potential errors
    • Some components were using less optimal React imports

  2. What is the improvement/solution?

    • Removed ESLint ignores for Avatar components
    • Properly typed imageProps to include data-testid property
    • Changed React.forwardRef to destructured { forwardRef } from React
    • Fixed string concatenation in error messages
    • Added missing default case in AvatarGroupStory component
    • Fixed TailwindCSS class ordering

Related issues

Part of: #657

Manual testing steps

  1. Run the storybook for design-system-react
  2. Verify all Avatar components render properly
  3. Verify tests pass for all Avatar components

Pre-merge author checklist

  • I've followed MetaMask Contributor Docs
  • I've completed the PR template to the best of my ability
  • I've included tests if applicable
  • I've documented my code using JSDoc format if applicable
  • I've applied the right labels on the PR

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@georgewrmarshall georgewrmarshall self-assigned this May 19, 2025
georgewrmarshall
georgewrmarshall commented May 19, 2025
View reviewed changes
georgewrmarshall
georgewrmarshall commented May 19, 2025
View reviewed changes
eslint.config.mjs
Comment on lines -25 to -30
'packages/design-system-react/src/components/AvatarFavicon/AvatarFavicon.test.tsx',
'packages/design-system-react/src/components/AvatarGroup/AvatarGroup.stories.tsx',
'packages/design-system-react/src/components/AvatarGroup/AvatarGroup.test.tsx',
'packages/design-system-react/src/components/AvatarGroup/AvatarGroup.tsx',
'packages/design-system-react/src/components/AvatarNetwork/AvatarNetwork.test.tsx',
'packages/design-system-react/src/components/AvatarToken/AvatarToken.test.tsx',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing avatars from file ignores in eslint config, this also enables automatic linting fixes like import order

georgewrmarshall
georgewrmarshall commented May 19, 2025
View reviewed changes
packages/design-system-react/src/components/AvatarFavicon/AvatarFavicon.test.tsx
@@ -65,7 +66,7 @@ describe('AvatarFavicon', () => {
<AvatarFavicon
name="ACME"
src={src}
imageProps={{ 'data-testid': 'img', id: 'img-id' } as any}
imageProps={{ 'data-testid': 'img', id: 'img-id' }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't be using a type assertion here (as any) to bypass a TypeScript type error. The reason for this is that ComponentProps<'img'> (the standard React type for <img> props) does not accept custom data-* attributes like data-testid by default, even though they are valid HTML attributes and commonly used in testing.

This is a valid type violation because the component's imageProps should allow test IDs and similar data-* attributes, but the current type definition doesn't support them. To properly fix this, we are explicitly extending the imageProps type in our avatar types.

georgewrmarshall
georgewrmarshall commented May 19, 2025
View reviewed changes
packages/design-system-react/src/components/AvatarFavicon/AvatarFavicon.types.ts
Comment on lines +24 to +26
imageProps?: ComponentProps<'img'> & {
'data-testid'?: string;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this comment https://github.com/MetaMask/metamask-design-system/pull/674/files#r2094696645

georgewrmarshall
georgewrmarshall commented May 19, 2025
View reviewed changes
packages/design-system-react/src/components/AvatarGroup/AvatarGroup.stories.tsx
@@ -2,6 +2,7 @@ import type { Meta, StoryObj } from '@storybook/react';
import React from 'react';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prettier fixes

georgewrmarshall
georgewrmarshall commented May 19, 2025
View reviewed changes
packages/design-system-react/src/components/AvatarGroup/AvatarGroup.test.tsx
variant={'Invalid' as any}
avatarPropsArr={[{ foo: 'bar' } as any]}
variant={'Invalid' as unknown as AvatarGroupVariant}
// @ts-expect-error - invalid variant
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we are expecting an error so @ts-expect-error is valid to ignore the type violation

georgewrmarshall
georgewrmarshall commented May 19, 2025
View reviewed changes
packages/design-system-react/src/components/AvatarGroup/AvatarGroup.tsx Outdated
@@ -1,22 +1,28 @@
import React, { useCallback } from 'react';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yarn lint:fix automatic fixes after being removed from ignore list

georgewrmarshall
georgewrmarshall commented May 19, 2025
View reviewed changes
packages/design-system-react/src/components/AvatarNetwork/AvatarNetwork.test.tsx
@@ -65,7 +66,7 @@ describe('AvatarNetwork', () => {
<AvatarNetwork
name="ACME"
src={src}
imageProps={{ 'data-testid': 'img', id: 'img-id' } as any}
imageProps={{ 'data-testid': 'img', id: 'img-id' }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same fixes for all imageProps types see this comment https://github.com/MetaMask/metamask-design-system/pull/674/files#r2094696645

@georgewrmarshall georgewrmarshall marked this pull request as ready for review May 19, 2025 04:12
@georgewrmarshall georgewrmarshall requested a review from a team as a code owner May 19, 2025 04:12
brianacnguyen
brianacnguyen previously approved these changes May 21, 2025
View reviewed changes
@georgewrmarshall georgewrmarshall merged commit 185d598 into main May 22, 2025
36 checks passed
@georgewrmarshall georgewrmarshall deleted the fix/dsr-lint-avatars branch May 22, 2025 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brianacnguyen brianacnguyen brianacnguyen approved these changes

Assignees

@georgewrmarshall georgewrmarshall

Labels
team-design-system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@georgewrmarshall @brianacnguyen