-
Notifications
You must be signed in to change notification settings - Fork 15
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
fix: create errors orders and fix translations #4202
base: master
Are you sure you want to change the base?
Conversation
0a51101
to
63cbde7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work @Nortsova 🥇 Thanks for fixing up the missing translations, it was something that was bugging me too for some time 😅
From what I've tested all error messages are now showing up in the right order thanks to your solution of using the context for registering the inputs order 🌟
Spam of screenshots on the way
And the input field descriptions also had a tooltip
Screen.Recording.2025-01-30.at.10.10.34.mov
Left a few refactoring comments, but nothing to block the merging of this PR, amazing work! 💯
src/i18n/en.json
Outdated
"errors.reputation.smite.notEnoughPoints": "Amount of reputation to remove is greater than the current reputation.", | ||
"errors.recipient.required": "Recipient is required", | ||
"errors.recipient.requiredIn": "Recipient is required in {paymentIndex, selectordinal, one {#st} two {#nd} few {#rd} other {#th}} payment.", | ||
"errors.percent.required": "Percent is required in {paymentIndex, selectordinal, one {#st} two {#nd} few {#rd} other {#th}} payment.", | ||
"errors.recipient.requiredIn": "Recipient is required in {paymentIndex, selectordinal, one {#st} two {#nd} few {#rd} other {#th}} payment", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename this to errors.recipient.requiredInPayment
for more clarity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really nice solution! 🌟
const { inputsOrder } = useInputsOrderContext(); | ||
|
||
const sortedFlatFormErrors = flatFormErrors.sort((a, b) => { | ||
const aIndex = inputsOrder.findIndex((fieldName) => a.key === fieldName); | ||
const bIndex = inputsOrder.findIndex((fieldName) => b.key === fieldName); | ||
|
||
if (aIndex === -1 && bIndex === -1) return 0; | ||
if (aIndex === -1) return 1; | ||
if (bIndex === -1) return -1; | ||
|
||
return aIndex - bIndex; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an improvement, we could create a Map
for the inputsOrder
to store the fieldName and the current index. This will help reduce repeated lookups through the inputsOrder
to find the index
of a given field. In the sort
function, if the fieldName
doesn't exist in the map, we can fallback to Infinity
and should still get the expected result.
const orderMap = new Map<string, number>(
inputsOrder.map((fieldName, index) => [fieldName, index]),
);
const sortedFlatFormErrors = flatFormErrors.sort((a, b) => {
const aIndex = orderMap.get(a.key.toString()) ?? Infinity;
const bIndex = orderMap.get(b.key.toString()) ?? Infinity;
return aIndex - bIndex;
});
This is more of a suggestion, so let me know what you think about it @Nortsova 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point! thank you :)
3b8e652
to
fcc4630
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job! I like this solution! Approving as it does resolve the original issue, I've got a few observations I'll leave but they're not necessarily directly related so feel free to decide if you think they are worth tackling here.
The biggest simple change that stood out to me is that all the fields have popovers now except for "Action type" and the "Token" field on the unlock token action. It would be nice to add those just for the sake of consistency.
I was able to get some wrong invalid errors showing. This only happens if you have the form open on "Staged payments" then switch to "Create agreement" and submit the form. If you directly open "Create agreement" via the sidebar, or select it after another action it isn't an issue.
Also on the create agreement form, "description" is a required field but the placeholder does not go red when submitting.
And lastly there is a little inconsistency with the "Member" field on the "Manage reputation" form. The field is disabled so when the form is submitted, the label stays greyed out whilst the select changes to red. I think the intended behaviour here is for the select to also stayed greyed out whilst it is disabled.
Nice job resolving the error order issue though, not a straightforward issue to solve!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work cleaning this up. I've tested all that I could, with most of them working. The only two that I had problems with are Split Payments and Manage Verified Members (details below)
Working:
- Simple Payment
- Advanced Payments
- Staged Payments
- Transfer funds
- Mint Tokens
- Unlock Tokens
- Manage Tokens
- Create new domain
- Edit an existing domain
- Manage reputation
- Manage permissions
- Arbitrary TX
- Edit Colony
- Create agreement
Not working
…rrors and split payment table errors
fcc4630
to
76f133a
Compare
Thanks, @rdig , you were right; split payments were working incorrectly; I added Regarding Manage verified members it is a bit complecated, it actually was a correct behaviour, because after user select method add or remove members table will appear, so last error was related to this hidden table. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good now. Nicely done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Nortsova really amazing efforts on this issue 🙌
I've retested the scenarios that were previously mentioned and it seems they behave properly 💯
However @Nortsova I did encounter an issue when switching from Staged payments
to Manage verified members
, when the createdIn
field appears as required and you can no longer submit the form 😢
Description
Testing
Step 1. Install voting reputation
Step 2. Install staged payments
Step 3. Open all action forms one by one and submit it to trigger validation
Step 4. Ensure that all form errors are consistent and shows in the same order as inputs
Step 5. Test that cursor pointer appears over the input labels:
Diffs
New stuff ✨
Changes 🏗
Resolves #3737
Resolves #3743