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

(WIP) Migrate SharedStyle to SharedStyle V2 #1466

Closed
wants to merge 12 commits into from

Conversation

hrithik73
Copy link
Contributor

  • Migrate the whole app to use SharedStylingV2

Copy link

vercel bot commented Mar 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
push-dapp ❌ Failed (Inspect) Apr 4, 2024 9:15am

Copy link

  • In the copyToClipboard function, the condition if (navigator && navigator.clipboard) should be improved by adding a check for the clipboard property as well, like if (navigator && navigator.clipboard && navigator.clipboard.writeText).
  • In the SpanV2 styled component, the property font-weight should be changed to fontWeight for consistency with other CSS properties in styled-components.
  • There is a typo in the fontWeight property in the SpanV2 styled component. It should be fontWeight instead of font-weight.
  • The 'background' CSS property in the DropdownItemContainer styled component has an extra quotation mark wrapping the color value. It should be background: red; without the quotes.
  • The width property is defined twice in the A styled component. It should be removed to avoid redundancy.

All other aspects of the code seem fine with logic and structure.

Therefore, the final assessment for the code review is:

All looks good.

@hrithik73 hrithik73 changed the title (WIP) Migrate to SharedStyle V2 (WIP) Migrate SharedStyle to SharedStyle V2 Mar 24, 2024
@hrithik73 hrithik73 requested a review from corlard3y March 27, 2024 08:28
@hrithik73 hrithik73 force-pushed the sharedstyle-migration branch from ea746dc to 3f32dd3 Compare March 28, 2024 10:27
loading={true}
height={13}
width={4}
/>
<H3
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change this H3 to H2V2 or SpanV2 according to V2 styling.


// Internal Configs
import GLOBALS from 'config/Globals';
import { nothing } from 'immer';
import { AppContext } from 'contexts/AppContext';
import { Span } from './SharedStyling';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Span is used. It should change to SpanV2 from V2 styling.

import SearchFilter from 'components/SearchFilter';
import { convertAddressToAddrCaip } from 'helpers/CaipHelper';
import CryptoHelper from 'helpers/CryptoHelper';
import { Item } from 'primaries/SharedStyling';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Item is still used. Change this according to V2 styling.

Copy link

github-actions bot commented Apr 4, 2024

In AliasProcessing.js:

  1. Typo in the import statement - 'styled' is imported twice. Remove the second import.
  2. There are syntax errors in the Tab component. The attributes should be inside curly braces.
  3. In the Tab component, the 'ItemHere' is not defined. It should be wrapped in a styled component or imported.
  4. In the Step component, the conditional css declaration seems to be missing a closing curly brace.
  5. In the Step component, there is a typo in the css property 'border-radius: 13px;'. It should be 'border-radius: 7px;'.
  6. In the Line component, 'z-index: -1;' may not have the desired effect. Review if this is intended functionality.

In AliasSetup.js:

  1. In the SpanV2 component, 'margin="5px 0px 60px 0px"' is not inside JSX braces.
  2. In the FadeLoader component, 'color="#cf1c84"' should be 'color="#cf1c84"'.
  3. In the ViewNFTs component, the map function should check if 'NFTObjects' has length before using it.
  4. In the ViewNFTs component, the key prop in the ViewNFTItem component should use a unique identifier from the NFTObject.

All looks good.

Copy link

github-actions bot commented Apr 4, 2024

In the AliasProcessing.js file:

  1. The Tab components are not structured correctly. The attributes should be wrapped in a single object.
  2. There are missing opening and closing braces for the Tab components.
  3. In the Step component, the conditional logic for styling based on type should be fixed.

In the AliasSetup.js file:

  1. There is a typo in 'Compoonents', change it to 'Components'.
  2. A typo in 'marginLeft' attribute in SpanV2 component.

In the AllNFTs.js file:

  1. A typo in 'import LoaderSpinner' should be 'import LoaderSpinner from'.

In the AllNFTsV2.js file:

  1. A similar issue as in AllNFTs.js with 'import LoaderSpinner'.

Please address the mentioned issues in the code files.

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.

SharedStylingV2 should be used everywhere instead of SharedStyling
3 participants