Skip to content

feat: add AssignmentTurnedInIcon to Sistent#1493

Open
Akshay000000 wants to merge 2 commits intolayer5io:masterfrom
Akshay000000:feature/add-assignmentturnedin-icon
Open

feat: add AssignmentTurnedInIcon to Sistent#1493
Akshay000000 wants to merge 2 commits intolayer5io:masterfrom
Akshay000000:feature/add-assignmentturnedin-icon

Conversation

@Akshay000000
Copy link
Copy Markdown

Notes for Reviewers

This PR fixes #1488

Adds AssignmentTurnedInIcon to the Sistent icon library to reduce dependency on @mui/icons-material and ensure design consistency across Layer5 products as required by Meshery's ConnectionChip component.

Signed commits

  • Yes, I signed my commits.

Signed-off-by: Akshay000000 <akshaysriram.b@gmail.com>

q	modified:   src/icons/index.ts
wq

Signed-off-by: Akshay000000 <akshaysriram.b@gmail.com>
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces the AssignmentTurnedInIcon component and updates several project dependencies in package-lock.json. The review feedback highlights a potential issue regarding the reclassification of core dependencies like React and MUI, which could lead to bundling conflicts in consumer applications. Additionally, it is recommended to update the new icon implementation to include a fill prop for better styling consistency and flexibility.

Comment thread package-lock.json
"version": "9.0.0",
"resolved": "https://registry.npmjs.org/@mui/core-downloads-tracker/-/core-downloads-tracker-9.0.0.tgz",
"integrity": "sha512-uwQNGkhv0lf7ufxw6QXev77BW6pWbW+7uxYjU5+rfp4lBkFtMEgJCsarTM3Tn+i0lGx6+Ol2u88JdGXr0GDskA==",
"dev": true,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Several core packages (e.g., react, @mui/material, @mui/icons-material) have had their "dev": true flag removed in the lockfile. This typically indicates they have been moved from devDependencies to dependencies in package.json. For a library, these should remain as peerDependencies and devDependencies to avoid bundling multiple instances of React or MUI in the consumer's application. Please verify that no accidental changes were made to the dependency types in package.json.

Comment on lines +1 to +20
import { DEFAULT_HEIGHT, DEFAULT_WIDTH } from '../../constants/constants';
import { IconProps } from '../types';

export const AssignmentTurnedInIcon = ({
width = DEFAULT_WIDTH,
height = DEFAULT_HEIGHT,
...props
}: IconProps): JSX.Element => {
return (
<svg
xmlns="http://www.w3.org/2000/svg"
viewBox="0 0 24 24"
width={width}
height={height}
{...props}
>
<path d="M19 3h-4.18C14.4 1.84 13.3 1 12 1c-1.3 0-2.4.84-2.82 2H5c-1.1 0-2 .9-2 2v14c0 1.1.9 2 2 2h14c1.1 0 2-.9 2-2V5c0-1.1-.9-2-2-2zm-7 0c.55 0 1 .45 1 1s-.45 1-1 1-1-.45-1-1 .45-1 1-1zm-2 14l-4-4 1.41-1.41L10 14.17l6.59-6.59L18 9l-8 8z" />
</svg>
);
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The icon implementation should destructure the fill prop and apply it to the path element, defaulting to DEFAULT_FILL_NONE (which is 'currentColor'). This ensures the icon respects the color prop (via inheritance) and allows for explicit fill overrides, maintaining consistency with standard React icon patterns.

import { DEFAULT_FILL_NONE, DEFAULT_HEIGHT, DEFAULT_WIDTH } from '../../constants/constants';
import { IconProps } from '../types';

export const AssignmentTurnedInIcon = ({
  width = DEFAULT_WIDTH,
  height = DEFAULT_HEIGHT,
  fill = DEFAULT_FILL_NONE,
  ...props
}: IconProps): JSX.Element => {
  return (
    <svg
      xmlns="http://www.w3.org/2000/svg"
      viewBox="0 0 24 24"
      width={width}
      height={height}
      {...props}
    >
      <path fill={fill} d="M19 3h-4.18C14.4 1.84 13.3 1 12 1c-1.3 0-2.4.84-2.82 2H5c-1.1 0-2 .9-2 2v14c0 1.1.9 2 2 2h14c1.1 0 2-.9 2-2V5c0-1.1-.9-2-2-2zm-7 0c.55 0 1 .45 1 1s-.45 1-1 1-1-.45-1-1 .45-1 1-1zm-2 14l-4-4 1.41-1.41L10 14.17l6.59-6.59L18 9l-8 8z" />
    </svg>
  );
};

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the review! I've added the fill prop with DEFAULT_FILL_NONE default and restored the package-lock.json. Please take another look.

Signed-off-by: Akshay000000 <akshaysriram.b@gmail.com>
@mascot-five
Copy link
Copy Markdown
Contributor

Thanks!

Copy link
Copy Markdown
Member

@rishiraj38 rishiraj38 left a comment

Choose a reason for hiding this comment

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

LGTM !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Add AssignmentTurnedInIcon to Sistent

3 participants