geido commented on code in PR #33054:
URL: https://github.com/apache/superset/pull/33054#discussion_r2044319718
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx:
##########
@@ -142,7 +158,24 @@ const HorizontalFormItem = styled(StyledFormItem)`
}
.ant-form-item-control {
- width: ${({ theme }) => theme.gridUnit * 41}px;
+ width: ${({ theme, inverseSelection }) =>
+ inverseSelection ? theme.gridUnit * 41 * 1.6 : theme.gridUnit * 41}px;
+ }
+
+ .ant-form-item-control-input-content {
+ ${({ inverseSelection }) => inverseSelection && 'display: flex;'}
Review Comment:
Same as my comment above, can we use the Ant Design built-in components such
as `Space` and `Flex`?
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx:
##########
@@ -118,9 +120,23 @@ const VerticalFormItem = styled(StyledFormItem)`
}
}
}
+
+ .ant-form-item-control-input-content {
+ ${({ inverseSelection }) => inverseSelection && 'display: flex;'}
Review Comment:
Can we avoid adding custom styles here and use a Flex component inside the
SelectFilterPlugin itself?
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -288,18 +309,54 @@ export default function PluginFilterSelect(props:
PluginFilterSelectProps) {
setDataMask(dataMask);
}, [JSON.stringify(dataMask)]);
+ useEffect(() => {
+ dispatchDataMask({
+ type: 'filterState',
+ extraFormData: getSelectExtraFormData(
+ col,
+ filterState.value,
+ !filterState.value?.length,
+ excludeFilterValues && inverseSelection,
+ ),
+ filterState: {
+ ...(filterState as {
+ value: SelectValue;
+ label?: string;
+ excludeFilterValues?: boolean;
+ }),
+ excludeFilterValues,
+ },
+ });
+ }, [excludeFilterValues]);
+
+ const handleExclusionToggle = (value: string) => {
+ setExcludeFilterValues(value === 'true');
+ };
+
return (
<FilterPluginStyle height={height} width={width}>
<StyledFormItem
validateStatus={filterState.validateStatus}
extra={formItemExtra}
>
+ {inverseSelection && (
+ <SelectWrapper>
+ <Select
+ value={`${excludeFilterValues}`}
+ options={[
+ { value: 'true', label: 'is not' },
Review Comment:
Should labels be localized?
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx:
##########
@@ -142,7 +158,24 @@ const HorizontalFormItem = styled(StyledFormItem)`
}
.ant-form-item-control {
- width: ${({ theme }) => theme.gridUnit * 41}px;
+ width: ${({ theme, inverseSelection }) =>
+ inverseSelection ? theme.gridUnit * 41 * 1.6 : theme.gridUnit * 41}px;
+ }
+
+ .ant-form-item-control-input-content {
+ ${({ inverseSelection }) => inverseSelection && 'display: flex;'}
+ }
+
+ .select-container {
+ ${({ inverseSelection, theme }) =>
+ inverseSelection &&
+ `
+ width: ${theme.gridUnit * 41}px;
Review Comment:
No `gridUnit` for widths, please. I know this was like this before but it is
likely a mistake.
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx:
##########
@@ -142,7 +158,24 @@ const HorizontalFormItem = styled(StyledFormItem)`
}
.ant-form-item-control {
- width: ${({ theme }) => theme.gridUnit * 41}px;
+ width: ${({ theme, inverseSelection }) =>
+ inverseSelection ? theme.gridUnit * 41 * 1.6 : theme.gridUnit * 41}px;
Review Comment:
width should not use `gridUnit` which should be only meant for spacing. Can
we use a fixed value here? Also, do we really need to set a specific width?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
