-
Notifications
You must be signed in to change notification settings - Fork 11.4k
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
[explorer + ts-sdk] Search input string type checks #2028
Conversation
this should close #2022 |
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.
Thanks for improving the search! In addition to the inline comments, could you also add a unit test for the added SDK util functions?
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 to see improved error handling. We need the 2 error pages to be rationalised to 1. Could we also have some tests set up to ensure the checks on search terms behave as intended?
Co-authored-by: Chris Li <76067158+666lcz@users.noreply.github.com>
Co-authored-by: Chris Li <76067158+666lcz@users.noreply.github.com>
Co-authored-by: Chris Li <76067158+666lcz@users.noreply.github.com>
i'm unifying the error pages right now, let's not merge yet. |
@@ -21,6 +26,24 @@ function getPlaceholderText(category: SearchCategory) { | |||
} | |||
} | |||
|
|||
function isInputValid(category: SearchCategory, input: string): boolean { | |||
if (overrideTypeChecks) return true; |
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.
I want to get rid of this but it requires overhauling the test data, i think this works fine for now
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.
Yes - we can revisit this when we move from static JSON to that delivered by the API.
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.
Excellent - this is ready for merge. It's also great to see Search Disambiguation between transactions and objects/addresses implemented at the search query level.
This adds some basic string type checks to the TypeScript SDK, for object ID, address, and transaction digest.
We use these new string type checks in the explorer in order to: