-
Notifications
You must be signed in to change notification settings - Fork 64
Improve typing for react states #967
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
base: main
Are you sure you want to change the base?
Conversation
| ); | ||
| const formErrorSignal = React.useRef< | ||
| Signal<Dialog<any>, boolean> | undefined | ||
| >(undefined); |
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.
These refs are pretty well-typed already. useRef's default for initialValue is undefined.
| props.formErrorSignalPromise?.promise, | ||
| props.formErrorSignalPromise | ||
| ? props.formErrorSignalPromise.promise | ||
| : Promise.resolve(undefined), |
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 looks equivalent to the previous code; is that the case?
| }); | ||
|
|
||
| let FormComponent; | ||
| let FormComponent: React.ComponentType<any>; |
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.
💯
| (() => { | ||
| /* no action needed on cancel */ | ||
| }) | ||
| } |
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 bit confused about this change :) Do we need this?
| okBtn.setAttribute('disabled', ''); | ||
| } else { | ||
| okBtn.removeAttribute('disabled'); | ||
| } |
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 can go back to the way it was. This change checks if (invalid) when we already know invalid is truthy:
if (invalid) {
// ...
if (invalid) {
// We already know this is true
} else {
// We handle the else case just below
}
} else {
//...
}| } | ||
|
|
||
| private okSignal: Signal<Dialog<any>, number>; | ||
| private okSignal: Signal<Dialog<IDict>, number>; |
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.
👍
| const [selectedLayer, setSelectedLayer] = useState<string | null>(null); | ||
| const [componentToRender, setComponentToRender] = useState<any>(null); | ||
| const [componentToRender, setComponentToRender] = | ||
| useState<JSX.Element | null>(null); |
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.
💯
| useState<JSX.Element | null>(null); | ||
|
|
||
| let LayerSymbology: React.JSX.Element; | ||
| let LayerSymbology: React.JSX.Element | null = null; |
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.
We can leave this as it was. componentToRender starts off as null, so it makes sense for its type to be JSX.Element | null, but LayerSymbology will always be a JSX.Element thanks to the case statement around line 87
|
Thanks Annie! TODOs: Rebase so only commit 8350d2a is included. Let me know if I can help! |
|
Integration tests report: appsharing.space |
8350d2a to
8bca1bc
Compare
Description
Fixed type errors in ProcessingFormDialog and SymbologyDialog components.
Updated useRef hooks with proper typing to avoid any and undefined issues.
Checklist
Resolves #XXX.Failing lint checks can be resolved with:
pre-commit run --all-filesjlpm run lint📚 Documentation preview: https://jupytergis--967.org.readthedocs.build/en/967/
💡 JupyterLite preview: https://jupytergis--967.org.readthedocs.build/en/967/lite