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

Challenge 0 wagmi 2 backmerge #171

Merged
merged 21 commits into from
May 15, 2024

Conversation

rin-st
Copy link
Collaborator

@rin-st rin-st commented Apr 24, 2024

No description provided.

Copy link
Collaborator

@Pabl0cks Pabl0cks left a comment

Choose a reason for hiding this comment

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

TY @rin-st it's looking nice, GJ!

Deploy link
Contract link

Just a couple of issues to check (nitpicks in this Challenge, but more to check if we got some bug to fix in SE-2 wagmiV2):

  • When clicking on "Transfers" tab we get this error: Error: Contract not found, but the information loads correctly.
  • When you paste an address on the "Transfer To" field for the first time after loading the "My NFTs" tab, we get a small flickering. Video here

@rin-st
Copy link
Collaborator Author

rin-st commented Apr 25, 2024

When clicking on "Transfers" tab we get this error: Error: Contract not found, but the information loads correctly.

You mean error in console, right? Added a quick fix for now. But looks like it's an at least 5-month old error, will add pr to se-2 soon

When you paste an address on the "Transfer To" field for the first time after loading the "My NFTs" tab, we get a small flickering. Video here

It's because of skeleton loading - input becomes wider, and hence card too. You can try to use ens names inside input, you'll see that card becomes wide. It worked that way already, I don't see the good way to fix it. We can fix width of the image so it will not widen on ens loading. So it will look like
image

instead of
image

@rin-st rin-st mentioned this pull request Apr 25, 2024
@rin-st
Copy link
Collaborator Author

rin-st commented Apr 25, 2024

created an issue for image width fix, I think it's better to fix it a bit later #172

@rin-st
Copy link
Collaborator Author

rin-st commented Apr 25, 2024

useScaffoldEventHistory update pr (could be merged later)

@Pabl0cks
Copy link
Collaborator

You mean error in console, right? Added a quick fix for now. But looks like it's an at least 5-month old error, will add pr to se-2 soon

Yeah! Just checked the SE-2 PR 🔥♥. Since we are still with Ch. 0, should we wait to merge it, and then backmerge it to base & chal0 so is fixed on all challenges in this PR iteration?

It's because of skeleton loading - input becomes wider, and hence card too.

created an issue for image width fix, I think it's better to fix it a bit later #172

Ok! It was a very nitpicky thing, only commented it because it seemed to me that in an older version of Chal. 0 it was not happening, if not I wouldn't have raised it here probably 🙌

@rin-st
Copy link
Collaborator Author

rin-st commented Apr 26, 2024

Yeah! Just checked the SE-2 PR 🔥♥. Since we are still with Ch. 0, should we wait to merge it, and then backmerge it to base & chal0 so is fixed on all challenges in this PR iteration?

Yes, let's wait then

Ok! It was a very nitpicky thing, only commented it because it seemed to me that in an older version of Chal. 0 it was not happening, if not I wouldn't have raised it here probably 🙌

It exists since start :)

* feat: update events hook and static params

* fix: deployed contracts
…2-challenges into challenge-0-wagmi-2-backmerge
@rin-st
Copy link
Collaborator Author

rin-st commented Apr 28, 2024

Yeah! Just checked the SE-2 PR 🔥♥. Since we are still with Ch. 0, should we wait to merge it, and then backmerge it to base & chal0 so is fixed on all challenges in this PR iteration?

Added events hook change and scaffold-eth/scaffold-eth-2#825 . Working fine for me, hope for you too 😄

I think I'll merge it again together with challenges 1 and 2

@Pabl0cks
Copy link
Collaborator

Added events hook change and scaffold-eth/scaffold-eth-2#825 . Working fine for me, hope for you too 😄

Working well to me!

I think I'll merge it again together with challenges 1 and 2

💯

@rin-st
Copy link
Collaborator Author

rin-st commented May 1, 2024

Added last changes, tested. Working fine.

@Pabl0cks
Copy link
Collaborator

Pabl0cks commented May 2, 2024

I'm getting a strange render problem with the copy address button with the "View QR Code" option from RainbowKit.

If I view QR from a Metamask wallet, it's fine, but if I'm connected to burner wallet, copy icon renders super small:

image

@Pabl0cks
Copy link
Collaborator

Pabl0cks commented May 2, 2024

The challenge is still working fine to me. Should we change the spacing in AddressQRCodeModal.tsx from space-x-4 to 1 or 0 to fix that kind of edge cases?

If we decide to tweak something, I'd probably just do it in se-2 and backmerge it in the next big backmerge to challenges, since is a really minor bug.

@rin-st
Copy link
Collaborator Author

rin-st commented May 3, 2024

I'd probably just do it in se-2 and backmerge it in the next big backmerge to challenges, since is a really minor bug.

You can create pr to se-2 and we'll add it to the current backmerge. Looks like we need to fix other bugs related to wagmi 2 in the current backmerge anyway

@Pabl0cks
Copy link
Collaborator

Pabl0cks commented May 3, 2024

You can create pr to se-2 and we'll add it to the current backmerge. Looks like we need to fix other bugs related to wagmi 2 in the current backmerge anyway

Tried reproducing it in SE-2, it's not happening for the same address. The problem is a combination of Challenges font and some longer weight addresses due to their characters size.

Should we change spacing a bit in SE-2 to fix that edge case (and other posible builds with SE-2 and big weight fonts), or should just leave it like that.

@rin-st
Copy link
Collaborator Author

rin-st commented May 3, 2024

I think it's better to fix it in se-2

@Pabl0cks
Copy link
Collaborator

Pabl0cks commented May 7, 2024

Did last tests, looking good! 🙌

@Pabl0cks
Copy link
Collaborator

While doing a fast check with the last burner commits on the challenges, realized of a SE-2 issue with the burner, a bit edge case, but maybe is good to add the fix when we have it.

@rin-st
Copy link
Collaborator Author

rin-st commented May 15, 2024

Merging this since the burner issue is closed. Thank you a lot @Pabl0cks ! ❤️

@rin-st rin-st merged commit 71bf930 into challenge-0-simple-nft May 15, 2024
@rin-st rin-st deleted the challenge-0-wagmi-2-backmerge branch May 15, 2024 11:13
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.

None yet

3 participants