-
Notifications
You must be signed in to change notification settings - Fork 377
chore(TextInputGroup): convert examples and demos to typescript= #6704
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
chore(TextInputGroup): convert examples and demos to typescript= #6704
Conversation
8b45f67 to
b49903a
Compare
| Attributes can be deselected (returning you to attribute selection mode) by hitting `escape`, or by hitting `backspace` when the only text in the text input is the attribute. | ||
|
|
||
| ```js file="./examples/TextInputGroup/AttributeValueFiltering.js" | ||
| ```js file="./examples/TextInputGroup/AttributeValueFiltering.tsx" |
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.
ts file I think
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.
hm... it may work both ways? aren't .ts files just for files with type declarations? I think .tsx could be correct? Now I'm second guessing everything lol
| When only one item remains in the suggestion list, tab can be used to auto-complete the typing of that item. | ||
|
|
||
| ```js file="./examples/TextInputGroup/AutoCompleteSearch.js" | ||
| ```js file="./examples/TextInputGroup/AutoCompleteSearch.tsx" |
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.
ts file here too
| menuRef.current && | ||
| !menuRef.current.contains(event.target) && | ||
| !textInputGroupRef.current.contains(event.target) | ||
| !menuRef.current.contains(event.target as Node) && |
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'm not used to seeing this type. Does it work if it's React.ReactNode? Maybe that's what I'm more used to seeing?
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.
It may also be HTMLElement? It may not be wrong either - I think we just tend to be a little more specific...
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.
TypeScript complains about React.ReactNode, but HTMLElement does work since it's an extension of an extension of Node. Pushing this change shortly.
nicolethoen
left a comment
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.
LGTM
|
After the conversation about these conversions yesterday I went to update all of the examples and demos in this component to have explicit function component return types, and in doing so ran into an issue with eslint when it comes to the demos. For the demos specifically, eslint throws an error for the no-anonymous-functions rule. It's specifically for the demos because an override was previously added which disables that rule in examples folders, but the demos folder is structured differently. The clearest solution to this issue in my opinion is to extend the eslint override to ignore files in the demo folder, so that's what I will do unless anyone objects. Other approaches would be to |
|
As was discussed, and in regards to consistency that was mentioned during Wednesday's stand up, I'm thinking the file names for these separated examples should follow a consistent format such as So instead of |
thatblindgeye
left a comment
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.
Looks good in terms of the updated example/file names!
kmcfaul
left a comment
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.
lgtm!
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #6679
Additional issues: