#6933: Enhance IconSymbolizer and Contrast enhancement #1
Conversation
dsuren1
changed the title
dsuren1
changed the title
style.ts
Outdated
| export interface VendorOption { | ||
| algorithm?: 'StretchToMinimumMaximum' | 'ClipToMinimumMaximum' | 'ClipToZero'; | ||
| minValue?: number; | ||
| maxValue?: number; | ||
| } |
There was a problem hiding this comment.
VendorOption seems a generic name but in this case is used only for ContrastEnhancement and it has a specific structure that match what the GeoServer expect.
Probably we should make this more specific:
export interface ContrastEnhancementVendorOption {
algorithm?: 'StretchToMinimumMaximum' | 'ClipToMinimumMaximum' | 'ClipToZero';
minValue?: number;
maxValue?: number;
}export interface ContrastEnhancementAlgorithm {
name?: 'StretchToMinimumMaximum' | 'ClipToMinimumMaximum' | 'ClipToZero';
minValue?: number;
maxValue?: number;
}
export interface ContrastEnhancement {
enhancementType?: 'normalize' | 'histogram';
gammaValue?: number;
algorithm?: ContrastEnhancementAlgorithm;
}or more generic to be used in other context
export interface VendorProperty {
name?: string;
options?: any;
}From the previous code it seems they prefer specific naming. My suggestions were more about the meaning of VendorOption itself that in this context seems to generic
There was a problem hiding this comment.
We agreed to change only the name of VendorOption to ContrastEnhancementVendorOption
style.ts
Outdated
| * A VendorOption defines the algorithm to apply for Normalize contrast enhancement. | ||
| */ | ||
| export interface VendorOption { | ||
| algorithm?: 'StretchToMinimumMaximum' | 'ClipToMinimumMaximum' | 'ClipToZero'; |
There was a problem hiding this comment.
I see that you are using nullable types for all the properties. If any of this is required I would remove the question mark.
There was a problem hiding this comment.
For example, it is strange to see a sum type (alghoritm) as nullable, I would add a 'None' type if no alghoritm is possible, and remove the nullable
There was a problem hiding this comment.
I think the algorithm property alone is required. So I will update the property accordingly
style.ts
Outdated
| export interface VendorOption { | ||
| algorithm?: 'StretchToMinimumMaximum' | 'ClipToMinimumMaximum' | 'ClipToZero'; | ||
| minValue?: number; | ||
| maxValue?: number; | ||
| } |
There was a problem hiding this comment.
We agreed to change only the name of VendorOption to ContrastEnhancementVendorOption
Description
This PR extends the IconSymbolizer and ContrastEnhancement interface with new properties
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x", remove the others)
##Issue
What is the current behavior?
#6933 & #1453
What is the new behavior?
Breaking change
Does this PR introduce a breaking change? (check one with "x", remove the other)
Other useful information