Skip to content

Conversation

@malvpatel
Copy link
Contributor

Addresses the Issue #57

return;
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I like providing error feedback to the user. I'm inclined to accept this even though it does expose
some issues.

In theory the json message can be translated. This method requires that the structure of the message
(value occurs before key and both are quoted strings) doesn't change. It also requires that there is only
one message that can be returned with a matching structure. If either of those requirements are not met,
I expect

this.popupRef.document.getElementsByName(field).item(0);

will not match anything and it looks like your code handles that case. That would lead to a missing
error message.

This means different feedback (UI) for the same issue depending on the language. IIRC the REST interface messages you are using are not currently translated. So this isn't an issue at the moment. It does create a
close coupling between a human readable (vs. machine readable) message generated from
the back end and the correct opertation of the classhelper popup. This will require some additional
care.

Please add a test to @BharathKanama test file. Have the test rigger an error that causes this code to
run and verify that the error message is displayed. You will probably need to log in as the demo user
and do a search expected to trigger the error.

One other area for translation is the use of:

span.textContent = `Invalid value: ${value}`;

This is a phrase and isn't as simple as translating a word. E.G. in some language this might have
to be expressed with a structure like "Value ${value} is not correct".
Adding the literal string: Invalid value: ${value} to the translation template and then expanding the
result should work. But this is more complex than just single word translations which have been
used up to this point and exposes some aditional risk.

So I say keep this code as is for the time being.

@malvpatel
Copy link
Contributor Author

@rouilj Should I merge the changes?

@rouilj
Copy link
Contributor

rouilj commented May 18, 2024

Yes, please merge #62

@malvpatel malvpatel merged commit 285f340 into main May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants