Skip to content

Conversation

@lissavxo
Copy link
Collaborator

Related to #427

Description

Fixed disabled behavior following this instructions

Test plan

Run the project with yarn dev or yarn clean:build
Using the generator
1 - Create a button active disabled option, the button text should be blurred
2 - Create a widget active disabled option, the widget and button text should be blurred
3 - Create a widget active disabled option, add goal amount, the goal amount bar should be blurred too
4 - Test open dialog buttons in the dev/demo/index.html there are two buttons and one is disabled, the disabled button should not open the dialog and the text should change to unavailable when click

@lissavxo lissavxo requested review from Klakurka and chedieck and removed request for chedieck November 25, 2024 16:14
@lissavxo lissavxo force-pushed the fix/disabled-behavior branch from 775a9ac to 65c43b2 Compare November 25, 2024 16:16
Klakurka
Klakurka previously approved these changes Nov 26, 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.

There's some "interesting" behavior depending on what settings you go with but this is fine IMO.

<Typography className={errorMsg ? classes.error : classes.text}>
{errorMsg
? errorMsg
: disabled
Copy link
Collaborator

@chedieck chedieck Nov 26, 2024

Choose a reason for hiding this comment

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

maybe some parenthesis and indentation here to make it more readable? The change makes the beginning more readable, but the part in the middle is quite hard to follow

@lissavxo lissavxo dismissed Klakurka’s stale review November 26, 2024 18:34

The merge-base changed after approval.

Klakurka
Klakurka previously approved these changes Nov 26, 2024
@lissavxo lissavxo dismissed Klakurka’s stale review November 26, 2024 21:51

The merge-base changed after approval.

Klakurka
Klakurka previously approved these changes Nov 27, 2024
@lissavxo lissavxo dismissed Klakurka’s stale review November 27, 2024 18:53

The merge-base changed after approval.

chedieck
chedieck previously approved these changes Nov 30, 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.

Tested, the described behavior applies therefore I'm approving, but:

Why are we blurring the disabled button but not the dialog? I actually don't understand the purpose of the "open payment dialog" (which was already present on master), AFAICT it acts just like a PayButton.

@lissavxo lissavxo dismissed chedieck’s stale review November 30, 2024 23:06

The merge-base changed after approval.

@chedieck chedieck self-requested a review November 30, 2024 23:08
chedieck

This comment was marked as spam.

@lissavxo lissavxo dismissed chedieck’s stale review November 30, 2024 23:42

The merge-base changed after approval.

@lissavxo lissavxo requested review from Klakurka and chedieck December 3, 2024 17:15
chedieck
chedieck previously approved these changes Dec 3, 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.

IDK why this dismissal is happening, approving again

@lissavxo lissavxo dismissed chedieck’s stale review December 3, 2024 21:02

The merge-base changed after approval.

@lissavxo lissavxo force-pushed the fix/disabled-behavior branch from dc657bf to f262a66 Compare December 3, 2024 21:07
@Klakurka Klakurka merged commit 55c9b04 into master Dec 3, 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