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]: Technical: add types for layer panel components #1819
Conversation
Signed-off-by: Evgeny Zhgulev <evgeny.zhgulev@actionengine.com>
Signed-off-by: Evgeny Zhgulev <evgeny.zhgulev@actionengine.com>
da2869e
to
1d7bed2
Compare
Signed-off-by: Evgeny Zhgulev <evgeny.zhgulev@actionengine.com>
Signed-off-by: Evgeny Zhgulev <evgeny.zhgulev@actionengine.com>
Signed-off-by: Evgeny Zhgulev <evgeny.zhgulev@actionengine.com>
Signed-off-by: Evgeny Zhgulev <evgeny.zhgulev@actionengine.com>
}; | ||
|
||
export type DatasetTagProps = { | ||
id?: string; | ||
dataset: KeplerTable; | ||
updateTableColor?: (d: string, c?: RGBColor) => void; | ||
updateTableColor?: typeof VisStateActions.updateTableColor; |
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.
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.
fixed
onClick?: (e: MouseEvent) => void; | ||
onClickSquare?: (e: MouseEvent) => void; | ||
}; | ||
|
||
export type ShowDataTableProps = { | ||
id: string; | ||
showDatasetTable: Function; | ||
showDatasetTable?: typeof VisStateActions.showDatasetTable; |
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.
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.
fixed
}; | ||
|
||
export type RemoveDatasetProps = { | ||
datasetKey: string; | ||
removeDataset?: (d: string) => void; | ||
removeDataset?: typeof openDeleteModal; |
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.
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.
fixed
showDeleteDataset: boolean; | ||
onTitleClick?: () => void; | ||
updateTableColor?: (d: string, c?: RGBColor) => void; | ||
removeDataset?: (d: string) => void; | ||
showDatasetTable?: typeof VisStateActions.showDatasetTable; |
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.
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.
fixed
onTitleClick?: () => void; | ||
updateTableColor?: (d: string, c?: RGBColor) => void; | ||
removeDataset?: (d: string) => void; | ||
showDatasetTable?: typeof VisStateActions.showDatasetTable; |
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.
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.
fixed
@@ -202,7 +230,7 @@ export const PaletteConfig = ({label, value, config: {type, options}, onChange}) | |||
{type === 'select' && ( | |||
<div className="color-palette__config__select"> | |||
<ItemSelector | |||
selectedItems={value} | |||
selectedItems={[value]} |
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.
shouldn't need to convert to array ItemSelector
props.selectedItems can be a single object
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.
fixed
@@ -226,12 +259,19 @@ const StyledColorRange = styled.div.attrs({ | |||
} | |||
`; | |||
|
|||
export const ColorPaletteGroup = ({reversed, onSelect, selected, colorRanges}) => ( | |||
export const ColorPaletteGroup = ({ |
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.
export const ColorPaletteGroup: React.FC<ColorPaletteGroupProps>
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.
fixed
layerOrder: number[]; | ||
layerClasses: LayerClassesType; | ||
showDeleteDataset: boolean; | ||
removeDataset: typeof UiStateActions.openDeleteModal; |
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.
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.
fixed
defaultDataset: string; | ||
showDatasetList: boolean; | ||
showDeleteDataset: boolean; | ||
showDatasetTable: typeof VisStateActions.showDatasetTable; |
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.
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.
fixed
@@ -35,7 +51,7 @@ const DimensionScaleSelector = ({label, onSelect, options, scaleType, disabled = | |||
</PanelLabel> | |||
<ItemSelector | |||
disabled={disabled} | |||
selectedItems={scaleType} | |||
selectedItems={scaleType ? [scaleType] : []} |
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.
shouldn't need to convert to array. selectedItems
can be a single object or an array of objects
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.
fixed
}[]; | ||
isDraggable?: boolean; | ||
openModal: typeof toggleModal; | ||
layerColorUIChange: typeof VisStateActions.layerColorUIChange; |
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.
use ActionHandler<>
#1801 (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.
fixed
@@ -132,7 +146,7 @@ function LayerPanelFactory(LayerConfigurator, LayerPanelHeader) { | |||
isVisible={config.isVisible} | |||
label={config.label} | |||
labelRCGColorValues={config.dataId ? datasets[config.dataId].color : null} | |||
layerType={layer.type} | |||
layerType={layer.type!} |
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.
what's the !
here? is it a typo?
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.
layer should have type defined, isn't it?
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's not always the case, a layer when first created, type might be null
, LayerPanelHeader
should handle layerType: null
case
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.
ok, fixed
]); | ||
const selectedItems = useMemo(() => { | ||
const selected = typeOptions.find(op => op.id === layer.type); | ||
return selected ? [selected] : []; |
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.
selectedItems doesn't have to be an array
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.
fixed
@@ -110,7 +122,7 @@ function TextLabelPanelFactory(RangeSlider, LayerConfigGroup, FieldSelector) { | |||
</PanelLabel> | |||
<ItemSelector | |||
{...LAYER_TEXT_CONFIGS.textAnchor} | |||
selectedItems={tl.anchor} | |||
selectedItems={[tl.anchor]} |
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.
same as above
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.
fixed
Signed-off-by: Evgeny Zhgulev <evgeny.zhgulev@actionengine.com>
Signed-off-by: Evgeny Zhgulev <evgeny.zhgulev@actionengine.com>
@@ -178,16 +192,16 @@ class ItemSelector extends Component<ItemSelectorProps> { | |||
// only used when multiSelect = true | |||
e.preventDefault(); | |||
e.stopPropagation(); | |||
const {selectedItems} = this.props; |
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.
due to ts cannot understand that selectedItems should be array in this case. Btw, in some cases it can be wrong - we can use multiSelect = true, but selectedItems not array type
@@ -120,7 +132,7 @@ function TextLabelPanelFactory(RangeSlider, LayerConfigGroup, FieldSelector) { | |||
</PanelLabel> | |||
<ItemSelector | |||
{...LAYER_TEXT_CONFIGS.textAlignment} | |||
selectedItems={tl.alignment} | |||
selectedItems={[tl.alignment]} |
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.
keep it as selectedItems={tl.alignment}
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.
fixed
type TextLabelPanelProps = { | ||
fields: Field[]; | ||
textLabel: LayerTextLabel[]; | ||
// updateLayerTextLabel: ( |
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.
remove comment out code
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.
removed
function PanelViewListToggleFactory(ToggleOption) { | ||
const PanelViewListToggle = props => { | ||
function PanelViewListToggleFactory(ToggleOption: ReturnType<typeof ToggleOptionFactory>) { | ||
const PanelViewListToggle = (props: PanelViewListToggleProps) => { |
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.
use React.FC<PanelViewListToggleProps>
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.
fixed
layerTypeOptions, | ||
onSelect, | ||
datasets | ||
}: LayerTypeSelectorProps) => { |
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.
use React.FC<LayerTypeSelectorProps>
@@ -59,7 +68,7 @@ const StyledListItem = styled.div` | |||
`; | |||
|
|||
export function LayerTypeListItemFactory() { | |||
const LayerTypeListItem = ({value, isTile}) => ( | |||
const LayerTypeListItem = ({value, isTile}: LayerTypeListItemProps) => ( |
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.
use React.FC
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.
fixed
@@ -68,7 +81,7 @@ export function LayerTypeDropdownListFactory() { | |||
selectedItems, | |||
selectionIndex, | |||
customListItemComponent | |||
}) => { | |||
}: LayerTypeDropdownListProps) => { |
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.
use React.FC
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.
fixed
layerId, | ||
label, | ||
onEdit | ||
}: { |
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.
use React.FC
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.
fixed
const WrappedSortableContainer = SortableContainer(({children}) => { | ||
return <div>{children}</div>; | ||
}); | ||
const LayerList = props => { | ||
|
||
const LayerList = (props: LayerListProps) => { |
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.
use React.FC
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.
fixed
layer: props.layer, | ||
fields: getLayerFields(props.datasets, props.layer), | ||
onChange: props.updateLayerConfig, | ||
setColorUI: props.updateLayerColorUI | ||
}); | ||
|
||
export const getVisConfiguratorProps = props => ({ | ||
export const getVisConfiguratorProps = (props: LayerConfiguratorProps) => ({ |
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.
use React.FC
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 function returns an object with props.
layer: props.layer, | ||
fields: getLayerFields(props.datasets, props.layer), | ||
onChange: props.updateLayerVisConfig, | ||
setColorUI: props.updateLayerColorUI | ||
}); | ||
|
||
export const getLayerChannelConfigProps = props => ({ | ||
export const getLayerChannelConfigProps = (props: LayerConfiguratorProps) => ({ |
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.
use React.FC
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 function returns an object with props.
@@ -123,7 +135,7 @@ export function LayerConfigGroupLabelFactory(InfoHelper) { | |||
} | |||
`; | |||
|
|||
const LayerConfigGroupLabel = ({label, description}) => ( | |||
const LayerConfigGroupLabel = ({label, description}: {label?: string; description?: string}) => ( |
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.
use React.FC
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.
fixed
@@ -42,7 +55,7 @@ function LayerColumnConfigFactory(ColumnSelector) { | |||
updateLayerConfig, | |||
assignColumn, | |||
assignColumnPairs | |||
}) => { | |||
}: LayerColumnConfigProps) => { |
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.
use React.FC
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.
fixed
options, | ||
scaleType, | ||
disabled = false | ||
}: DimensionScaleSelectorProps) => { |
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.
use React.FC
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.
fixed
@@ -67,17 +67,16 @@ const StyledColorBlock = styled.div.attrs({ | |||
flex-grow: 1; | |||
`; | |||
|
|||
const ColorPalette = ({colors, height, className, isSelected, isReversed}) => ( | |||
const ColorPalette = ({colors, height, className, isSelected, isReversed}: ColorPaletteProps) => ( |
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.
use React.FC
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.
fixed
@@ -99,7 +107,7 @@ function AddLayerButtonFactory() { | |||
</ListItemWrapper> | |||
); | |||
|
|||
const AddLayerButton = props => { | |||
const AddLayerButton = (props: AddLayerButtonProps) => { |
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.
use React.FC
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.
fixed
@@ -104,7 +129,7 @@ const LazyInput = ({value, onChange, name}) => { | |||
); | |||
}; | |||
|
|||
const CustomInput = ({isRanged, value, onChangeCustomInput}) => { | |||
const CustomInput = ({isRanged, value, onChangeCustomInput}: CustomInputProps) => { |
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.
use React.FC
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.
fixed
@@ -161,7 +174,7 @@ export default function VisConfigSliderFactory(RangeSlider) { | |||
disabled, | |||
onChange, | |||
inputTheme | |||
}) => { | |||
}: VisConfigSliderProps) => { |
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.
use React.FC
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.
fixed
const VisConfigSwitch = ({ | ||
layer: {id, config}, | ||
property, | ||
onChange, | ||
label, | ||
description, | ||
disabled | ||
}) => ( | ||
}: VisConfigSwitchProps) => ( |
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.
use React.FC
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.
fixed
Signed-off-by: Evgeny Zhgulev <evgeny.zhgulev@actionengine.com>
@heshan0131 can we merge it? Looks like all comments are resolved |
Signed-off-by: Evgeny Zhgulev evgeny.zhgulev@actionengine.com