Skip to content

Use foundry UI in sncast #3396

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

Open
wants to merge 22 commits into
base: 3022-2-message-components
Choose a base branch
from

Conversation

franciszekjob
Copy link
Contributor

@franciszekjob franciszekjob commented May 22, 2025

Towards #3022

Stack:

Introduced changes

All printed output from sncast is now done through foundry-ui

Checklist

  • Linked relevant issue
  • Updated relevant documentation
  • Added relevant tests
  • Performed self-review of the code
  • Added changes to CHANGELOG.md

@franciszekjob franciszekjob changed the title Use foundry-ui in sncast Use foundry UII in sncast May 23, 2025
@franciszekjob franciszekjob changed the title Use foundry UII in sncast Use foundry UI in sncast May 23, 2025
@franciszekjob franciszekjob marked this pull request as ready for review May 23, 2025 00:01
@franciszekjob franciszekjob requested a review from a team as a code owner May 23, 2025 00:01
@franciszekjob franciszekjob requested review from cptartur and kkawula and removed request for a team, cptartur and kkawula May 23, 2025 00:01
@franciszekjob franciszekjob marked this pull request as draft May 23, 2025 00:28
Copy link
Member

@cptartur cptartur left a comment

Choose a reason for hiding this comment

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

Integration looks simple enough, except for the number of arguments.

Sidenote, maybe the work for sncast and forge could be separate PRs. It's bit hard to follow in which parts where the most significant changes

Copy link
Member

Choose a reason for hiding this comment

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

General comment for all functions, it could make sense to refactor signatures of some of them so they don't take a million (plus a new one) arguments.

Maybe that should be part of the stack, but we can talk it through at the next meeting.

@franciszekjob
Copy link
Contributor Author

franciszekjob commented May 23, 2025

Sidenote, maybe the work for sncast and forge could be separate PRs. It's bit hard to follow in which parts where the most significant changes

That's what I'm doing but due to the fact that we're using shared functions across our crates, I had to apply some changes in other crates than sncast :/

@cptartur
Copy link
Member

Sidenote, maybe the work for sncast and forge could be separate PRs. It's bit hard to follow in which parts where the most significant changes

Thath's what I'm doing but due to the fact that we're using shared functions across our crates, I had to apply some changes in other crates than sncast :/

If that's best what we can do then okay.

@franciszekjob franciszekjob requested a review from cptartur May 26, 2025 08:38
@franciszekjob franciszekjob mentioned this pull request May 26, 2025
5 tasks
@franciszekjob franciszekjob requested a review from kkawula May 27, 2025 09:59
@franciszekjob franciszekjob marked this pull request as ready for review May 27, 2025 09:59
Copy link
Member

@cptartur cptartur left a comment

Choose a reason for hiding this comment

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

Probably is fine, didn't review in detail assuming it's all trivial stuff.

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.

2 participants