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

Refactor css, components #321

Merged
merged 7 commits into from
Dec 27, 2023
Merged

Refactor css, components #321

merged 7 commits into from
Dec 27, 2023

Conversation

skqksh
Copy link
Collaborator

@skqksh skqksh commented Dec 21, 2023

No description provided.

Copy link
Member

@jinoosss jinoosss left a comment

Choose a reason for hiding this comment

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

It looks good to me.
However, it seems better to use values from the theme injected from the provider rather than static data.
What do you think ?

https://github.com/onbloc/adena-wallet/blob/main/packages/adena-extension/src/app.tsx#L20


export const modeVariants = {
normal: css`
background: ${({ theme }): string => theme.color.neutral[6]};
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this theme static data?
Even if you have one current theme, it seems like a good idea to use the theme injected by the theme provider.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this a file in component/pages ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so.
that's because, 'ApproachPrivateKey.tsx' is not a component that is used repeatedly, but a part of the page.

Copy link
Member

Choose a reason for hiding this comment

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

I understand.
I think it's just a difference of opinion.
I think component -> page makes sense to separate UI and data even if it's not reused, for example when creating storybooks and jest files.

jinoosss
jinoosss previously approved these changes Dec 25, 2023
Copy link
Member

@jinoosss jinoosss left a comment

Choose a reason for hiding this comment

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

Looks good to me. 👍

@jinoosss
Copy link
Member

The UI of the screen showing the transaction history seems to be broken.
Is this something we should work on further ?

@skqksh
Copy link
Collaborator Author

skqksh commented Dec 26, 2023

The UI of the screen showing the transaction history seems to be broken. Is this something we should work on further ?

Thank you for checking the screen.
This problem is caused by a style conflict due to classname-based CSS inherited from the parent component.
To prevent similar issues, I will continue to refactor classname into styled-component.

Copy link
Member

@jinoosss jinoosss left a comment

Choose a reason for hiding this comment

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

👍

@dongwon8247 dongwon8247 merged commit 754745c into onbloc:main Dec 27, 2023
2 checks passed
@skqksh skqksh deleted the main branch December 28, 2023 09:04
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.

3 participants