Skip to content

fix: lint violations maskicon #680

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.

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

Merged
merged 2 commits into from
May 23, 2025
Merged

Conversation

georgewrmarshall
Copy link
Contributor

@georgewrmarshall georgewrmarshall commented May 21, 2025

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:

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

Screenshot 2025-05-22 at 2 06 16 PMScreenshot 2025-05-22 at 2 08 43 PM

React

Screenshot 2025-05-22 at 2 06 46 PM Screenshot 2025-05-22 at 2 08 30 PM

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 (see labeling guidelines). Not required for external contributors.

@@ -65,9 +62,6 @@ const config = createConfig([
'packages/design-system-react-native/src/components/temp-components/ImageOrSvg/ImageOrSvg.stories.tsx',
'packages/design-system-react-native/src/components/temp-components/ImageOrSvg/ImageOrSvg.test.tsx',
'packages/design-system-react-native/src/components/temp-components/ImageOrSvg/ImageOrSvg.tsx',
'packages/design-system-react-native/src/components/temp-components/Maskicon/Maskicon.test.tsx',
'packages/design-system-react-native/src/components/temp-components/Maskicon/Maskicon.tsx',
'packages/design-system-react-native/src/components/temp-components/Maskicon/Maskicon.utilities.ts',
'packages/design-system-react-native/src/components/temp-components/Spinner/Spinner.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 maskicon components from file ignore list in eslint config

@@ -31,6 +31,7 @@ module.exports = merge(baseConfig, {
'!**/*.dev.{js,ts}', // Exclude .dev files
'!**/*.assets.{js,ts}', // Exclude .assets files
'!**/*.types.{js,ts}', // Exclude .types files
'!./src/components/temp-components/Blockies/Blockies.utilities.d.ts',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolves this testing issue for new recent types file for blockies utilities

Screenshot 2025-05-21 at 12 51 25 PM

@georgewrmarshall georgewrmarshall force-pushed the fix/lint-violiations-maskicon branch from 9270213 to d0943c5 Compare May 22, 2025 04:40
@georgewrmarshall georgewrmarshall marked this pull request as ready for review May 22, 2025 04:41
@georgewrmarshall georgewrmarshall requested a review from a team as a code owner May 22, 2025 04:41
Copy link
Contributor

📖 Storybook Preview

@georgewrmarshall georgewrmarshall merged commit 77a0c6c into main May 23, 2025
37 checks passed
@georgewrmarshall georgewrmarshall deleted the fix/lint-violiations-maskicon branch May 23, 2025 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants