-
Notifications
You must be signed in to change notification settings - Fork 947
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
WIP React experiments #1796
WIP React experiments #1796
Conversation
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.
If these comments are not applicable to TypeScript because the tsx file or the usage, let's discuss them.
|
||
export | ||
class DropdownView extends DescriptionView { | ||
/** |
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.
Style nit: Could we do 2 spaces instead of 4?
/** | ||
* Called when view is rendered. | ||
*/ | ||
render() { |
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.
Explicit return type. And everywhere else this occurs.
this.update(); | ||
} | ||
|
||
update() { |
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.
Explicit return of void.
this.el.appendChild(this.listbox); | ||
this.update(); | ||
} | ||
|
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.
A JSDoc comment on public methods for improved typings.
labels: this.model.get('_options_labels'), | ||
selectedIndex: this.model.get('index'), | ||
id: this.label.htmlFor, | ||
handleChange: (event) => { |
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.
Define an explicit return type. This appears to be void.
import * as ReactDOM from 'react-dom'; | ||
|
||
import * as React from 'react'; | ||
|
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 a fan of a minimum class comment in JSDoc style. This will help if users/developers with types with helpful hints in an editor like VSCode.
ReactDOM.render(React.createElement(DropDown, props), this.listbox); | ||
} | ||
|
||
listbox: HTMLDivElement; |
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.
If this variable is used externally, add a JSDoc style comment above.
/**
* list box is an HTMLDivElement for foo bar.
*/
Thanks for the helpful comments. I'm still in the experimental phase, just trying to get the patterns right, and then will go back through and clean up the code. What do you think of the React pattern here? |
No problem. I need to really read up on React to be helpful. Disappears for a week to read up on it... |
Don't merge. This is just some experiments in using react.