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

feat: rfox improve no rewards address ux #8552

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

gomesalexandre
Copy link
Contributor

Description

Does what it says on the box, makes things more obvious for users re: no rewards address:

  • The first change which makes the rest possible here is to default to manual address input when we detect both no current rewards address and no wallet runtime/support.
    In this case, we effectively assume that manual address input is the default/only valid flow here. This may not be exactly true in case accounts are loading, but this is a compromise
    to make things work for most cases and reduce regressions risks, at the expense of this one specific case (users can always revert back to wallet account selection).
  • Makes the input required if there is no current rune address. This is made possible thanks to the above: the (manual) input form is only conditionally rendered if the conditions above are met.
    This means that native users will not end up there (unless explicitly selecting manual address input, in which case manual input will be required as expected), as they will default to account selection.
    For others (e.g EVM-only wallets), the input will be the default, and since it is required, will render default react-hook-form/browser red required styles.
  • Adds button translations for "Select a RUNE address", in case the red errored input state above is not obvious enough

Issue (if applicable)

closes #8487

Risk

High Risk PRs Require 2 approvals

What protocols, transaction types, wallets or contract interactions might be affected by this PR?

Low/Medium, RFOX form validation is quite a risky one

Testing

  • EVM wallets without a RUNE rewards address yet should default to manual input selection, with errored input styles and "Select RUNE address" copy until a RUNE addy is input
  • Native wallets accounts without RUNE rewards address (with accounts already loaded) should default to account selection, and never have the copy mentioned above nor input error since there is no input
    (unless explicitly selecting manual address input, in which case the input validation flow should be just the same as EVM wallets)
  • EVM/native wallet accounts with a rewards addy (assuming accounts loaded) should behave the same as develop

Engineering

  • ^

Operations

  • 🏁 My feature is behind a flag and doesn't require operations testing (yet)
  • ^

Screenshots (if applicable)

https://jam.dev/c/2ad3937d-0676-4788-b82b-ce2b39fcfca8

@gomesalexandre gomesalexandre requested a review from a team as a code owner January 13, 2025 14:18
useEffect(() => {
handleRuneAddressChange(undefined)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See onChange() below

@@ -116,7 +139,8 @@ export const AddressSelection: FC<AddressSelectionProps> = ({
return (
<Input
{...register('manualRuneAddress', {
minLength: 1,
// Only required if user doesn't have a rewards address yet
required: !currentRuneAddress,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

More of a paranoia than anything else.
We will not render the manual field input if currentRuneAddress (disabled account selection will be rendered instead, or a tag for EVM wallets), so we should be safe with required: true but better be safe than sorry by being explicit.

Comment on lines +190 to +194
onChange: e => {
if (!e.target.value) {
handleRuneAddressChange(undefined)
}
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reset on empty input

@@ -110,7 +110,7 @@ export const StakeInput: React.FC<StakeInputProps & StakeRouteProps> = ({

const methods = useForm<StakeInputValues>({
defaultValues: defaultFormValues,
mode: 'onChange',
mode: 'all',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i.e onBlur, onChange, onSubmit and onTouched, ensuring that the input becomes red if errorred in all those change modes.

Comment on lines +439 to +440
if (errors.manualRuneAddress?.type === 'required') return translate('RFOX.selectRuneAddress')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explicit handling of the required error, since it is not coming from a custom validation rule, but from native react-hook-form's required property above

!isAccountsMetadataLoading
Boolean(
errors.amountFieldInput ||
// Required rewards input isn't an error per se
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Avoids screaming at users with big scary red color for the required case

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.

Unable to stake into rfox on a wallet that has no existing rfox balance
1 participant