Skip to content

[ESLint] Fix ESLint violations in the currently ignored files for the design-system-react package #657

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
Open
georgewrmarshall opened this issue May 11, 2025 · 0 comments
Open

[ESLint] Fix ESLint violations in the currently ignored files for the design-system-react package #657

georgewrmarshall opened this issue May 11, 2025 · 0 comments
Assignees
@georgewrmarshall
Labels
team-design-system

Comments

@georgewrmarshall
Copy link
Contributor

georgewrmarshall commented May 11, 2025 •
edited
Loading

Description

Fix ESLint violations in the currently ignored files for the design-system-react package and remove them from the ignore list. This will improve code quality, type safety, and maintainability across the entire codebase.

Technical Details

Several files are currently ignored in the eslint.config.mjs file:

// design system react
'packages/design-system-react/src/components/Icon/icons/*.tsx',
'packages/design-system-react/src/components/Icon/icons/index.ts',
'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',
'packages/design-system-react/src/components/BadgeNetwork/BadgeNetwork.test.tsx',
'packages/design-system-react/src/components/BadgeWrapper/BadgeWrapper.test.tsx',
'packages/design-system-react/src/components/temp-components/Jazzicon/Jazzicon.test.tsx',
'packages/design-system-react/src/components/temp-components/Jazzicon/Jazzicon.tsx',
'packages/design-system-react/src/components/temp-components/Maskicon/Maskicon.test.tsx',
'packages/design-system-react/src/components/temp-components/Maskicon/Maskicon.tsx',
'packages/design-system-react/src/components/temp-components/Maskicon/Maskicon.utilities.ts',

Common violations include:

  • no-bitwise: Using bitwise operators like <<, &, and >>
  • @typescript-eslint/no-non-null-assertion: Using non-null assertion operator (!)
  • Import ordering issues
  • Type safety concerns

Approach

  1. Work through each file in the ignore list, addressing its specific ESLint violations
  2. Prioritize files with fewer or simpler violations first
  3. For components with bitwise operations (like Maskicon), consider:
    • Replacing with standard arithmetic operations where possible
    • Adding proper documentation where bitwise operations are necessary
    • Using ESLint exceptions with clear explanations only when absolutely needed
  4. For TypeScript non-null assertions, replace with proper null checking
  5. Remove each file from the ignore list as violations are fixed
  6. Ensure all tests continue to pass

Acceptance Criteria

  • All files are removed from the ignore list in eslint.config.mjs
  • All ESLint violations are fixed or have proper exceptions with explanations
  • No new ESLint warnings are introduced
  • All tests continue to pass
  • Code functionality remains unchanged

References

  • Current ESLint ignore list in eslint.config.mjs
  • Specific violations in Maskicon.utilities.ts serve as examples of issues to fix
@georgewrmarshall georgewrmarshall changed the title [ESLint] Fix id-length rule in design-system-react package [ESLint] Enable and Fix Disabled ESLint Rules in design-system-react Package May 11, 2025
@georgewrmarshall georgewrmarshall mentioned this issue May 13, 2025
Merged
7 tasks
@georgewrmarshall georgewrmarshall changed the title [ESLint] Enable and Fix Disabled ESLint Rules in design-system-react Package [ESLint] Fix ESLint violations in the currently ignored files for the design-system-react package May 13, 2025
@georgewrmarshall georgewrmarshall self-assigned this May 13, 2025
georgewrmarshall added a commit that referenced this issue May 14, 2025
@georgewrmarshall
fix: ESlint config (#630)
e461bd5 
## **Description**

This PR fixes eslint which has been broken since [Eslint v9
upgrade](#379) (3
months! 😱). It incorporates @mcmire's [work in
core](MetaMask/core#5132) to resolve the issue
and adds a script to prevent further eslint violations via the eslint
threshold script.

The solution is based on reviewing the history of Core's [eslint config
file](https://github.com/MetaMask/core/commits/main/eslint.config.mjs)
and the latest updates in main. Due to the lack of linting for over 3
months, there are many linting violations, so we have ignored files that
need fixes outside of automated fixing.

This is the first step in resolving our linting configuration. A
follow-up PR will address the ignored files in `design-system-react` and
`design-system-react-native` libraries.
- #657
- #658

### Key updates:

- Updated eslint.config.mjs to align with core
- Added @mcmire's eslint warning threshold script
- Removed individual package eslint configurations as they no longer
work for design-system-react and design-system-react-native
- Updated contributors docs
- Updated build pipeline to better work with tailwind prettier plugin
- Automated eslint fixes with yarn lint:fix

## **Related issues**

Fixes: #629

## **Manual testing steps**

1. Run `yarn lint:eslint` to verify linting now works correctly
2. Verify that the CI pipeline completes successfully with the updated
eslint configuration
3. Confirm that the eslint threshold script properly tracks and reports
violations

## **Screenshots/Recordings**

<!-- Not applicable for this PR -->

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs)
- [x] I've completed the PR template to the best of my ability
- [x] I've included tests if applicable
- [x] I've documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I've applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md))

## **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.
This was referenced May 17, 2025
Merged
Merged
Merged
Merged
georgewrmarshall added a commit that referenced this issue May 21, 2025
@georgewrmarshall
fix: import-x/no-named-as-default-member violations dsr (#673)
eea1e27 
## **Description**

This PR fixes ESLint `import-x/no-named-as-default-member` linting
errors throughout the design-system-react package by replacing all
instances of `React.forwardRef` with proper named imports.

The changes include:
1. Adding `{ forwardRef }` to React import statements in all component
files
2. Replacing `React.forwardRef` with `forwardRef` in component
implementations
3. Similarly addressing `React.createRef` in test files by importing and
using `{ createRef }`

These changes follow modern React best practices and improve code
consistency across the codebase. The improvements make the code more
maintainable and address the linting errors that were previously ignored
or generating warnings.

A script was created to automate this process, ensuring consistent
implementation across all affected files.

## **Related issues**

Part of: #657

## **Manual testing steps**

1. Run `yarn lint` to verify no more
`import-x/no-named-as-default-member` errors for React.forwardRef
2. Run unit tests to ensure component functionality remains unchanged
3. Build the project to verify no compilation errors

## **Screenshots/Recordings**

### Before
<img width="390" alt="Screenshot 2025-05-19 at 10 10 34 AM"
src="https://github.com/user-attachments/assets/5e5462eb-9797-45e8-83eb-b13d628386df"
/>

### After

<img width="594" alt="Screenshot 2025-05-19 at 10 09 22 AM"
src="https://github.com/user-attachments/assets/3dbab472-570e-497f-a7a8-cc0e796128d6"
/>

<img width="651" alt="Screenshot 2025-05-19 at 10 08 24 AM"
src="https://github.com/user-attachments/assets/5e88f374-5f98-4702-94b2-b20c114228ab"
/>

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs)
- [x] I've completed the PR template to the best of my ability
- [x] I've included tests if applicable
- [x] I've documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I've applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md))

## **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 added a commit that referenced this issue May 21, 2025
@georgewrmarshall
chore: fixing badge component linting (#675)
e220191 
## **Description**

This PR fixes linting violations in the BadgeNetwork and BadgeWrapper
components in the design-system-react package by:
1. Removing ESLint ignores for these components in the config
2. Improving test assertions using `toStrictEqual` instead of `toEqual`
3. Adding proper null checks before accessing parent elements
4. Implementing proper mock functions with return values
5. Adding the unicode flag to regex for better internationalization
support
6. Improving code comments and spacing

## **Related issues**

Part of: #657

## **Manual testing steps**

1. Run ESLint on the project to verify no linting errors
2. Run the tests to confirm all tests still pass
3. Verify the Badge components render correctly in Storybook

## **Screenshots/Recordings**

Not applicable for linting fixes.

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs)
- [x] I've completed the PR template to the best of my ability
- [x] I've included tests if applicable
- [x] I've documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I've applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **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 mentioned this issue May 21, 2025
Merged
5 tasks
georgewrmarshall added a commit that referenced this issue May 22, 2025
@georgewrmarshall
fix: dsr avatar linting violations (#674)
185d598 
## **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**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs)
- [x] I've completed the PR template to the best of my ability
- [x] I've included tests if applicable
- [x] I've documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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 added a commit that referenced this issue May 22, 2025
@georgewrmarshall
fix: auto fix eslint violations from ignore files (#672)
8415efa 
# **Description**

This PR temporarily uncomments the ESLint ignore rules for the design
system React Native components and runs `yarn lint:fix` to automatically
fix linting violations. This helps improve code quality and consistency
across the codebase.

The automated fixes include:
- Adding newlines between import groups
- Using proper type imports instead of value imports
- Using destructuring for object properties
- Replacing string concatenation with template literals
- Adding JSDoc comments to functions
- Removing unnecessary eslint-disable comments
- Fixing import ordering

# **Related issues**

Part of: #657 

# **Manual testing steps**

1. Check out this branch
2. Run `yarn lint` to verify that the linting errors are fixed
3. Run the storybook to make sure components still render correctly

# **Screenshots/Recordings**

N/A

# **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs)
- [x] I've completed the PR template to the best of my ability
- [x] I've included tests if applicable
- [x] I've documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I've applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md))

# **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 added a commit that referenced this issue May 23, 2025
@georgewrmarshall
fix: lint violations maskicon (#680)
77a0c6c 
## **Description**

This PR fixes linting violations in the Maskicon component across both
React and React Native versions of the design system. The changes are
focused on addressing various ESLint warnings and following best
practices to ensure code consistency.

Key improvements include:
1. Adopting proper test assertions using `toStrictEqual()` instead of
`toEqual()`
2. Proper handling of asynchronous code with `void`
3. Following consistent import ordering conventions
4. Adding appropriate ESLint comments instead of disabling rules broadly
5. Improving type safety in the test mocks
6. Removing the Maskicon components from the ESLint ignore list now that
violations are fixed

## **Related issues**

Part of: 
- #657
- #658

## **Manual testing steps**

1. Run `yarn lint` to verify linting violations are fixed
2. Run tests for the Maskicon component to ensure functionality still
works: `yarn test Maskicon`
3. Build the component to ensure it still renders correctly: `yarn
build`

## **Screenshots/Recordings**


### React Native

<img width="48" alt="Screenshot 2025-05-22 at 2 06 16 PM"
src="https://github.com/user-attachments/assets/666fa27e-127b-4f55-bba7-47c7759aafd1"
/><img width="58" alt="Screenshot 2025-05-22 at 2 08 43 PM"
src="https://github.com/user-attachments/assets/89fc7294-722d-4a59-b989-0e23c10e9fa0"
/>


### React 

<img width="446" alt="Screenshot 2025-05-22 at 2 06 46 PM"
src="https://github.com/user-attachments/assets/de17fe24-bdca-4510-a0bc-983c68271201"
/>
<img width="455" alt="Screenshot 2025-05-22 at 2 08 30 PM"
src="https://github.com/user-attachments/assets/25e93ca2-7b52-4fb8-b598-dddad320eb48"
/>

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs)
- [x] I've completed the PR template to the best of my ability
- [x] I've included tests if applicable
- [x] I've documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I've applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@georgewrmarshall georgewrmarshall

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

No branches or pull requests
1 participant
@georgewrmarshall