Skip to content
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

Design system fixes #1882

Merged
merged 7 commits into from
Oct 3, 2024
Merged

Design system fixes #1882

merged 7 commits into from
Oct 3, 2024

Conversation

abhishek-01k
Copy link
Collaborator

Pull Request Template

Ticket Number

Description

  • Fixed the given design system fixes.

  • Problem/Feature:

Type of Change

  • Bug fix
  • New feature
  • Code refactor
  • Documentation update
  • Other (please describe):

Checklist

  • Quick PR: Is this a quick PR? Can be approved before finishing a coffee.
    • Quick PR label added
  • Not Merge Ready: Is this PR dependent on some other PR/tasks and not ready to be merged right now.
    • DO NOT Merge PR label added

Frontend Guidelines

Build & Testing

  • No errors in the build terminal
  • Engineer has tested the changes on their local environment
  • Engineer has tested the changes on deploy preview

Screenshots/Video with Explanation

  • Before: Explain the previous behavior

  • After: What's changed now

Additional Context

Review & Approvals

  • Self-review completed
  • Code review by at least one other engineer
  • Documentation updates if applicable

Notes

Copy link

github-actions bot commented Oct 1, 2024

There are a couple of mistakes in the code provided:

  1. In both the Logout and Sale components, the stroke-width attribute should be strokeWidth in JSX, as it's camelCase in React.
  2. In the Logout component, the width="inherit" and height="inherit" attributes should be removed or replaced with specific values like width="24" and height="24".
  3. In the Sale component, the stroke-width="2" attribute should be strokeWidth="2".

Here is the corrected code:

import { FC } from 'react';
import { IconWrapper } from '../IconWrapper';
import { IconProps } from '../Icons.types';

const Logout: FC<IconProps> = (allProps) => {
  const { svgProps: props, ...restProps } = allProps;
  return (
    <IconWrapper componentName="Logout" icon={
      <svg
        xmlns="http://www.w3.org/2000/svg"
        width="24"
        height="24"
        viewBox="0 0 24 24"
        fill="none"
        {...props}
      >
        <path
          d="M10.0962 1H3.63464C2.53008 1 1.63464 1.89543 1.63464 3V21C1.63464 22.1046 2.53007 23 3.63464 23H10.0962"
          stroke="currentColor"
          strokeWidth="2"
          strokeLinecap="round"
        />
        <path
          d="M8.8269 11.5769H22.3654"
          stroke="currentColor"
          strokeWidth="2"
          strokeLinecap="round"
        />
        <path
          d="M17.7115 6.0769L22.3654 11.5769L17.7115 17.5"
          stroke="currentColor"
          strokeWidth="2"
          strokeLinecap="round"
        />
      </svg>
    }
    {...restProps}
    />
  );
};

export default Logout;

import { FC } from 'react';
import { IconWrapper } from '../IconWrapper';
import { IconProps } from '../Icons.types';

const Sale: FC<IconProps> = (allProps) => {
  const { svgProps: props, ...restProps } = allProps;
  return (
    <IconWrapper componentName="Sale" icon={
      <svg
        xmlns="http://www.w3.org/2000/svg"
        width="22"
        height="22"
        viewBox="0 0 22 22"
        fill="none"
        {...props}
      >
        <path
          d="M1.22222 10.7778C6.49717 10.7778 10.7778 6.49717 10.7778 1.22222C10.7778 1.09984 10.8776 1 11 1C11.1224 1 11.2222 1.09984 11.2222 1.22222C11.2222 6.49717 15.5028 10.7778 20.7778 10.7778C20.9002 10.7778 21 10.8776 21 11C21 11.1224 20.9002 11.2222 20.7778 11.2222C15.5028 11.2222 11.2222 15.5028 11.2222 20.7778C11.2222 20.9002 11.1224 21 11 21C10.8776 21 10.7778 20.9002 10.7778 20.7778C10.7778 15.5028 6.49717 11.2222 1.22222 11.2222C1.09984 11.2222 1 11.1224 1 11C1 10.8776 1.09984 10.7778 1.22222 10.7778Z"
          stroke="currentColor"
          strokeWidth="2"
        />
      </svg>
    }
    {...restProps}
    />
  );
};

export default Sale;

After the corrections, the code looks good.

Copy link

github-actions bot commented Oct 1, 2024

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-10-03 08:30 UTC

Copy link

github-actions bot commented Oct 1, 2024

In the Logout component:

  • The value of width attribute in the <svg> element should be "100%" instead of "inherit".
  • The value of height attribute in the <svg> element should be "100%" instead of "inherit".
  • Replace stroke-width="2" with strokeWidth={2} in all <path> elements.
  • Replace stroke-linecap="round" with strokeLinecap="round" in all <path> elements.

In the Sale component:

  • The value of width attribute in the <svg> element should be "100%" instead of "inherit".
  • The value of height attribute in the <svg> element should be "100%" instead of "inherit".
  • Replace stroke-width="2" with strokeWidth={2} in the <path> element.

Overall, the code structure and logic seem fine. After the mentioned corrections, the components should render as expected.

All looks good.

Copy link

github-actions bot commented Oct 1, 2024

  • In the Logout component, the stroke-width should be written as strokeWidth instead.
  • In the Sale component, the stroke-width should be written as strokeWidth instead.
  • In both components, the width attribute in the svg tag should be specified with a pixel value rather than inherit.
  • Check if the IconWrapper component is correctly receiving and handling the icon and componentName props.
  • The imports in the file seem fine.
All looks good.

Copy link

github-actions bot commented Oct 1, 2024

In the file src/blocks/icons/components/ArbitrumMonotone.tsx, there are a couple of mistakes:

  1. The path tag should have the attributes fill-rule and clip-rule inside it, not outside.
  2. In the path tag, some d attributes are missing in the opening tag.

Here is the corrected version:

import { FC } from 'react';
import { IconWrapper } from '../IconWrapper';
import { IconProps } from '../Icons.types';

const ArbitrumMonotone: FC<IconProps> = (allProps) => {
  const { svgProps: props, ...restProps } = allProps;
  return (
    <IconWrapper
      componentName="ArbitrumMonotone"
      icon={
        <svg
          width="inherit"
          height="inherit"
          viewBox="0 0 20 20"
          fill="none"
          xmlns="http://www.w3.org/2000/svg"
          {...props}
        >
          <g clip-path="url(#clip0_8547_11033)">
            <path
              d="M0 8C0 3.58172 3.58172 0 8 0H12C16.4183 0 20 3.58172 20 8V12C20 16.4183 16.4183 20 12 20H8C3.58172 20 0 16.4183 0 12V8Z"
              fill="var(--surface-tertiary)"
              fill-rule="evenodd"
              clip-rule="evenodd"
            />
          </g>
          <defs>
            <clipPath id="clip0_8547_11033">
              <path
                d="M0 8C0 3.58172 3.58172 0 8 0H12C16.4183 0 20 3.58172 20 8V12C20 16.4183 16.4183 20 12 20H8C3.58172 20 0 16.4183 0 12V8Z"
                fill="white"
              />
            </clipPath>
          </defs>
        </svg>
      }
      {...restProps}
    />
  );
};

export default ArbitrumMonotone;

For the src/blocks/icons/components/BnbMonotone.tsx file, there are similar mistakes as in ArbitrumMonotone file. Please correct them accordingly.

Once these corrections are done for both files, you can mark them as 'All looks good'.

Copy link

github-actions bot commented Oct 1, 2024

In the file src/blocks/Blocks.types.ts:

  1. There is a syntax error in the import statements for SeparatorResponsiveCSSProperties, SeparatorResponsiveCSSPropertiesData, SeparatorResponsivePropValues. They need to be wrapped in curly braces like other imports.

  2. The second '...' in the snippet should be removed as it doesn't belong there.

  3. The 'fill-rule' and 'clip-rule' attributes within the tag in the SVG are missing an '=' sign.

  4. In the 'CamelCase' type definition, the 'infer' keyword should be 'Extract'.

After these corrections, the rest of the code looks good.

Let me know if you need any more assistance.

src/blocks/tag/Tag.tsx Outdated Show resolved Hide resolved
src/blocks/tag/Tag.tsx Outdated Show resolved Hide resolved
src/blocks/tag/Tag.tsx Outdated Show resolved Hide resolved
@@ -29,4 +31,5 @@ export const breakpointMap: Record<Breakpoint | 'initial', DeviceSizeName | ''>
lp: 'laptop',
ll: 'laptopL',
dp: 'desktop',
uw: 'ultrawide',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Screenshot 2024-10-01 at 3 29 41 PM You also need to update this function so Box also supports this size.

<Box display={{uw:"flex"}}>

Copy link

github-actions bot commented Oct 1, 2024

In the file src/blocks/Blocks.types.ts:

  1. There is a syntax error in the imports section after importing 'SkeletonResponsivePropValues'. Remove the extra commas and place 'SeparatorResponsiveCSSProperties' import statement in a new line.
  2. There is an unfinished type declaration for 'CamelCase'. It seems like there should be a return type specified.
  3. The comment "This needs to be removed when the color dependency from Globals.js is removed." has no effect and can be removed.

In the file src/blocks/icons/components/ArbitrumMonotone.tsx:

  1. There is a syntax error in the second 'path' element. The attributes 'fill-rule' and 'clip-rule' are not enclosed in curly braces {}.

In the file src/blocks/icons/components/BnbMonotone.tsx:

  1. There is a syntax error in the 'd' attribute of the second 'path' element. The attribute is not enclosed in curly braces {}.

In the file src/blocks/icons/components/EtheriumMonotone.tsx:

  1. There are syntax errors in the 'd' attributes of the 'path' elements. They are not enclosed in curly braces {}.

In the file src/blocks/icons/components/Logout.tsx:

  1. There are syntax errors in the 'd' attributes of the 'path' elements. They are not enclosed in curly braces {}.

Overall, there are syntax errors that need to be fixed in the mentioned files. The corrections will involve enclosing attribute values in curly braces where necessary.

Copy link

github-actions bot commented Oct 1, 2024

In the file src/blocks/Blocks.types.ts:

  • There is a typo in the import statement for SeparatorResponsiveCSSProperties. It should be placed inside the braces.
  • The comment /* This needs to be removed when the color dependency from Globals.js is removed. */ ends abruptly and is incomplete.
  • It seems that the type CamelCase is defined but never used in the code.
  • There is an ellipsis (...) after declaring ResponsiveCSSPropertyData, which suggests that there might be more code following it that is not shown.

In the file src/blocks/Blocks.utils.ts:

  • There is a missing closing curly brace } for the getResponsiveCSS function right before the @param border.
  • Inside the getBlocksBorder function, the comment @param border seems incomplete. It should describe the purpose of the parameter.
  • There is a missing import { css } from 'styled-components'; at the beginning of the file for the css function used in the getResponsiveCSS function.

All looks good.

@rohitmalhotra1420 rohitmalhotra1420 merged commit 4b633d6 into main Oct 3, 2024
2 checks passed
@rohitmalhotra1420 rohitmalhotra1420 linked an issue Oct 3, 2024 that may be closed by this pull request
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.

Design system fixes
2 participants