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.

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

feat: Integrate safe react components #1602

Merged
merged 15 commits into from
Feb 6, 2023
Merged

feat: Integrate safe react components #1602

merged 15 commits into from
Feb 6, 2023

Conversation

yagopv
Copy link
Member

@yagopv yagopv commented Jan 26, 2023 •
edited
Loading

What it solves

Resolves 5afe/safe-react-components#198

How this PR fixes it

We are removing the theme files, fonts and EthHashInfo and ExplorerButton components from the web-core project and migrating them to a shared library.

This PR integrates the safe-react-components v2 implementing these changes

@github-actions
Copy link

github-actions bot commented Jan 26, 2023 •
edited
Loading

Branch preview

✅ Deploy successful!

https://feat_integrate_safe_react_components--webcore.review-web-core.5afe.dev

@github-actions
Copy link

github-actions bot commented Jan 26, 2023 •
edited
Loading

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total
Report generated by eslint-plus-action

@yagopv yagopv marked this pull request as ready for review January 31, 2023 09:17
iamacook
iamacook reviewed Jan 31, 2023
View reviewed changes
Copy link
Member

@iamacook iamacook left a comment

Choose a reason for hiding this comment

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

Great work! For consistency sake, I would suggest keeping the lightPalette name instead of renaming it.

src/components/common/EthHashInfo/index.tsx Outdated Show resolved Hide resolved
src/pages/_app.tsx Show resolved Hide resolved
@yagopv
Copy link
Member Author

yagopv commented Jan 31, 2023

Great work! For consistency sake, I would suggest keeping the lightPalette name instead of renaming it.

You mean the lightPalette as palette in several files? Yeah i can do it

@katspaugh
Copy link
Member

katspaugh commented Jan 31, 2023 •
edited
Loading

colors.ts was used in the css-vars.ts script (and the corresponding command in package.json) to generate CSS variables from the theme. Needs to be adjusted accordingly.

Ah, sorry, you changed it 👍

@yagopv
Copy link
Member Author

yagopv commented Jan 31, 2023

colors.ts was used in the css-vars.ts script (and the corresponding command in package.json) to generate CSS variables from the theme. Needs to be adjusted accordingly.

yep i did it, is working i think

iamacook
iamacook approved these changes Jan 31, 2023
View reviewed changes
scripts/css-vars.ts Outdated
import spacings from '../src/styles/spacings.js'

const { lightPalette: palette, darkPalette } = reactComponents
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can we change this to be lightPalette as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

changed! thanks

katspaugh
katspaugh requested changes Feb 3, 2023
View reviewed changes
Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

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

Looks like there's a visual regression in the EthHashInfo:

On this branch:
Screenshot 2023-02-03 at 07 30 59

On prod:
Screenshot 2023-02-03 at 07 32 21

@yagopv
Copy link
Member Author

yagopv commented Feb 3, 2023 •
edited
Loading

Looks like there's a visual regression in the EthHashInfo:

Ok I see, there is some 2.3em! important! width and heights not being applied in the lib. Will take a look

@yagopv yagopv mentioned this pull request Feb 3, 2023
Merged
@dasanra dasanra requested a review from katspaugh February 3, 2023 15:03
@yagopv
Copy link
Member Author

yagopv commented Feb 6, 2023

Looks like there's a visual regression in the EthHashInfo:

@katspaugh the issue is fixed

@yagopv yagopv merged commit 1b4f92c into dev Feb 6, 2023
@yagopv yagopv deleted the feat/integrate-safe-react-components branch February 6, 2023 11:25
@github-actions github-actions bot locked and limited conversation to collaborators Feb 6, 2023
@liliya-soroka
Copy link
Member

Verified

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

@dasanra dasanra dasanra approved these changes

@iamacook iamacook iamacook approved these changes

@katspaugh katspaugh katspaugh approved these changes

@DaniSomoza DaniSomoza Awaiting requested review from DaniSomoza

Assignees
No one assigned
Labels
None yet
Projects
Safe{Wallet}
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Revamp safe-react-components
5 participants
@yagopv @katspaugh @liliya-soroka @iamacook @dasanra