Skip to content

Conversation

@lissavxo
Copy link
Collaborator

Related to #426

Description

Fix opReturn fail behavior

Test plan

create a button/widget with op-return prop set to a large string like this:
"ABCDEFGHIJKLMNOPABCDEFlkfdnlkfndlksnkjsnfkldsnlknlksssssssss;sssssssssGHIJKLMNOPABCDEFGHIJKLMNOPABCDEFGHIJKLMNOPABCDEFGHIJKLMNOPABCDEFGHIJKLMNOPABCDEFGHIJKLMNOPABCDEFGHIJKLMNOPABCDEFGHIJKLMNOPABCDEFGHIJKLMNOP"
the expected behavior is blur QR code and show the error message in console instead of in the button text.

Copy link
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

  • Rebase to master.
  • Can we add this (OP_RETURN msg) to the button generator?

Copy link
Collaborator

@chedieck chedieck left a comment

Choose a reason for hiding this comment

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

The behavior described in the test plan checks out, however, the issue mentions reverting the text back, not getting ridding of it and using the console.

I don't think showing the error in the console only is a good idea, I think we should display that same text of before but having it cleared out after 5s (as we do in other places).

The color could be red to indicate an error, too.

chedieck
chedieck previously approved these changes Nov 18, 2024
Copy link
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

After a few seconds, the text at the top changes from the red error to "send any amount":

image

I get that this is kinda out of scope and related to #327 but IMO we should just deal with this now. Up to you if you want to do it in this branch or a new one.

chedieck
chedieck previously approved these changes Nov 19, 2024
Copy link
Collaborator

@chedieck chedieck left a comment

Choose a reason for hiding this comment

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

After a few seconds, the text at the top changes from the red error to "send any amount":

I thought this was the desired behavior, as per the issue description.

In any case, I also think is better to leave the error there.

@lissavxo lissavxo dismissed chedieck’s stale review November 20, 2024 00:11

The merge-base changed after approval.

@Klakurka
Copy link
Member

After a few seconds, the text at the top changes from the red error to "send any amount":

I thought this was the desired behavior, as per the issue description.

In any case, I also think is better to leave the error there.

It's referring to when the widget is regenerated.

Klakurka
Klakurka previously approved these changes Nov 20, 2024
Copy link
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

We can improve the error message at some point as the wording is pretty technical but it's fine for now.

@lissavxo lissavxo dismissed Klakurka’s stale review November 20, 2024 00:21

The merge-base changed after approval.

Klakurka
Klakurka previously approved these changes Nov 20, 2024
@lissavxo lissavxo dismissed Klakurka’s stale review November 20, 2024 14:17

The merge-base changed after approval.

@chedieck chedieck merged commit 1c35f2a into master Nov 26, 2024
1 check passed
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.

4 participants