Adding type to layer dialog hooks#937
Conversation
|
|
|
Integration tests report: appsharing.space |
packages/base/src/dialogs/symbology/components/color_ramp/ColorRamp.tsx
Outdated
Show resolved
Hide resolved
…in SingleBandPseudoColor, Categorized, and Graduated components
mfisher87
added
the
maintenance
…SingleBandPseudoColor and Graduated components
mfisher87
left a comment
mfisher87
left a comment
There was a problem hiding this comment.
I have to leave for a conference soon, so just leaving some thoughts for now :) I'd like to help out a bit more soon!
packages/base/src/dialogs/symbology/components/color_ramp/ModeSelectRow.tsx
Outdated
Show resolved
Hide resolved
packages/base/src/dialogs/symbology/components/color_ramp/ColorRamp.tsx
Outdated
Show resolved
Hide resolved
packages/base/src/dialogs/symbology/components/color_ramp/ModeSelectRow.tsx
Show resolved
Hide resolved
|
|
||
| useEffect(() => { | ||
| if (selectedRamp === '' && selectedMode === '' && numberOfShades === '') { | ||
| if (selectedRamp === '') { |
There was a problem hiding this comment.
Should we still check if selectedMode and numberofShades are undefined here?
There was a problem hiding this comment.
Sure we can check if selectedMode and numberofShades are undefined here as it is More defensive ensures all three are unset before populating.
packages/base/src/dialogs/symbology/tiff_layer/types/SingleBandPseudoColor.tsx
Outdated
Show resolved
Hide resolved
packages/base/src/dialogs/symbology/vector_layer/types/Graduated.tsx
Outdated
Show resolved
Hide resolved
packages/base/src/dialogs/symbology/vector_layer/types/Graduated.tsx
Outdated
Show resolved
Hide resolved
packages/base/src/dialogs/symbology/components/color_ramp/ColorRamp.tsx
Outdated
Show resolved
Hide resolved
| selectedMode: ClassificationMode | undefined, | ||
| numberOfShades: number | undefined, |
There was a problem hiding this comment.
| selectedMode: ClassificationMode | undefined, | |
| numberOfShades: number | undefined, | |
| selectedMode: ClassificationMode, | |
| numberOfShades: number, |
Let's make these required and check that these are defined before allowing the user to trigger this function to be called (e.g. don't allow the "Classify" button to be clicked unless these are defined). Then we can remove the casts below!
| selectedRamp: string; | ||
| selectedRamp: ColorRampName; |
There was a problem hiding this comment.
What do you think with this change. I think using ColorRampName instead of only type string is a good option.
|
@nakul-py this conflicts with your "default classes" PR that was merged, could you please rebase this and fix conflicts? |
|
Hey! @martinRenou I have resolved conflicts in |
* Adding type to hooks * Adding new ClassificationMode type * Refactor classification mode handling to use ClassificationMode type in SingleBandPseudoColor, Categorized, and Graduated components * Add validation for number of shades in ColorRamp and update usage in SingleBandPseudoColor and Graduated components * Refactor ColorRamp and mode components to improve type handling. * making selectedMode or numberofShades required * lint fix 🔔 * Removing duplicate export for * by mistake forget this file to commit
3 participants
Description
Adding typing to useState hooks and Disable the Classify button if user does not provide valid number or negative.
Checklist
Resolves #XXX.Failing lint checks can be resolved with:
pre-commit run --all-filesjlpm run lint📚 Documentation preview: https://jupytergis--937.org.readthedocs.build/en/937/
💡 JupyterLite preview: https://jupytergis--937.org.readthedocs.build/en/937/lite