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

Added Min Property to Fingerprint Duration input field #408

Merged
merged 3 commits into from
Mar 5, 2024

Conversation

code-Gambler
Copy link
Contributor

Description

Added the min property to the Fingerprint Duration input field

Related Issue

407

Does this introduce a breaking change?

  • Yes
  • No

Other information

…rintDuration is less that 0

Disabled the continue button because while the min property was in place there was still a loop hole
to enter a value less that 0

407
ran prettier:write

407
Copy link
Contributor Author

@code-Gambler code-Gambler left a comment

Choose a reason for hiding this comment

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

I also disabled the continue button if the value of fingerprintDuration is less that zero because if we only add the min property, there was a small loop where the user can enter a negative value and directly click on continue to submit a request.

Copy link
Contributor

@0xjei 0xjei left a comment

Choose a reason for hiding this comment

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

LGTM +1

Copy link
Member

@vplasencia vplasencia left a comment

Choose a reason for hiding this comment

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

Thank you so much! Great work! ✨

I left one comment.

@@ -206,7 +207,8 @@ export default function GeneralInfoStep({
_fingerprintDuration === undefined) ||
(group.type === "off-chain" && !_groupDescription) ||
(group.type === "off-chain" &&
_groupDescription.length < 10)
_groupDescription.length < 10) ||
_fingerprintDuration < 0
Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

We could add this:

Suggested change
_fingerprintDuration < 0
(group.type === "off-chain" && _fingerprintDuration < 0)

What do you think about adding this:

isDisabled={
                        !group.type ||
                        !_groupName ||
                        (group.type === "off-chain" &&
                            (!_groupDescription ||
                                _groupDescription.length < 10 ||
                                _fingerprintDuration === undefined ||
                                _fingerprintDuration < 0))
                    }

@vplasencia
Copy link
Member

I also disabled the continue button if the value of fingerprintDuration is less that zero because if we only add the min property, there was a small loop where the user can enter a negative value and directly click on continue to submit a request.

@code-Gambler Regarding this, it would be nice to disable the button when people type - and it shows NaN. We could add Number.isNaN(_fingerprintDuration) ||, then the result should be something like:

isDisabled={
                        !group.type ||
                        !_groupName ||
                        (group.type === "off-chain" &&
                            (!_groupDescription ||
                                _groupDescription.length < 10 ||
                                _fingerprintDuration === undefined ||
                                Number.isNaN(_fingerprintDuration) ||
                                _fingerprintDuration < 0))
                    }

Here is the new issue to add it: #414

What do you think?

@vplasencia vplasencia merged commit 9057ec7 into bandada-infra:dev Mar 5, 2024
2 checks 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.

3 participants