michael-s-molina commented on a change in pull request #17025:
URL: https://github.com/apache/superset/pull/17025#discussion_r735907198
##########
File path:
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx
##########
@@ -18,10 +18,12 @@
*/
import React, { useMemo } from 'react';
import { styled } from '@superset-ui/core';
-import { Form, FormItem } from 'src/components/Form';
+import { FormItem as StyledFormItem, Form } from 'src/components/Form';
+import { Tooltip } from 'src/components/Tooltip';
+import { theme as supersetTheme } from 'src/preamble';
Review comment:
```suggestion
```
##########
File path:
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx
##########
@@ -62,21 +58,56 @@ const StyledFilterControlContainer = styled(Form)`
}
`;
-const FormItem = styled(Form.Item)`
+const FormItem = styled(StyledFormItem)`
${({ theme }) => `
.ant-form-item-label {
- padding-bottom: ${theme.gridUnit}px;
+ label.ant-form-item-required:not(.ant-form-item-required-mark-optional){
+ &::after {
+ display: none;
+ }
+ }
}
-
- `}
+`}
`;
const ToolTipContainer = styled.div`
font-size: ${({ theme }) => theme.typography.sizes.m}px;
display: flex;
- padding-left: ${({ theme }) => theme.gridUnit}px;
`;
+const RequiredFieldIndicator = () => (
+ <span
+ css={{
+ color: supersetTheme.colors.error.base,
Review comment:
You shouldn't use the theme directly. The `css` prop accepts a function
that receives the `theme` as the first prop. You can import `SupersetTheme` as
the type of prop.
```
import { SupersetTheme } from '@superset-ui/core';
css={(theme: SupersetTheme) => ({...})}
```
##########
File path:
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx
##########
@@ -66,17 +119,22 @@ const FilterControl: React.FC<FilterProps> = ({
filter,
filter.dataMask?.filterState,
);
+ const isRequired = !!filter.controlValues?.enableEmptyFilter;
const label = useMemo(
() => (
<StyledFilterControlTitleBox>
<StyledFilterControlTitle data-test="filter-control-name">
{name}
</StyledFilterControlTitle>
+ {isRequired && <RequiredFieldIndicator />}
+ {filter.description && filter.description.trim() && (
+ <DescriptoinToolTip description={filter.description} />
Review comment:
```suggestion
<DescriptionToolTip description={filter.description} />
```
##########
File path:
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx
##########
@@ -50,8 +52,59 @@ const StyledFilterControlContainer = styled(Form)`
width: 100%;
padding-right: ${({ theme }) => theme.gridUnit * 11}px;
}
+ .ant-form-item-tooltip {
+ margin-bottom: ${({ theme }) => theme.gridUnit}px;
+ }
+`;
+
+const FormItem = styled(StyledFormItem)`
+ .ant-form-item-label {
+ label.ant-form-item-required:not(.ant-form-item-required-mark-optional) {
+ &::after {
+ display: none;
+ }
+ }
+ }
`;
+const ToolTipContainer = styled.div`
+ font-size: ${({ theme }) => theme.typography.sizes.m}px;
+ display: flex;
+`;
+
+const RequiredFieldIndicator = () => (
+ <span
+ css={{
+ color: supersetTheme.colors.error.base,
+ fontSize: `${supersetTheme.typography.sizes.s}px`,
+ paddingLeft: `${supersetTheme.gridUnit}px`,
+ }}
+ >
+ *
+ </span>
+);
+
+const DescriptoinToolTip = ({ description }: { description: string }) => (
Review comment:
```suggestion
const DescriptionToolTip = ({ description }: { description: string }) => (
```
##########
File path:
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx
##########
@@ -66,17 +119,22 @@ const FilterControl: React.FC<FilterProps> = ({
filter,
filter.dataMask?.filterState,
);
+ const isRequired = !!filter.controlValues?.enableEmptyFilter;
const label = useMemo(
() => (
<StyledFilterControlTitleBox>
<StyledFilterControlTitle data-test="filter-control-name">
{name}
</StyledFilterControlTitle>
+ {isRequired && <RequiredFieldIndicator />}
+ {filter.description && filter.description.trim() && (
Review comment:
I think it would be better to apply `trim` when saving to avoid saving a
bunch of blank spaces.
##########
File path:
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx
##########
@@ -62,21 +58,56 @@ const StyledFilterControlContainer = styled(Form)`
}
`;
-const FormItem = styled(Form.Item)`
+const FormItem = styled(StyledFormItem)`
${({ theme }) => `
.ant-form-item-label {
- padding-bottom: ${theme.gridUnit}px;
+ label.ant-form-item-required:not(.ant-form-item-required-mark-optional){
+ &::after {
+ display: none;
+ }
+ }
}
-
- `}
+`}
`;
const ToolTipContainer = styled.div`
font-size: ${({ theme }) => theme.typography.sizes.m}px;
display: flex;
- padding-left: ${({ theme }) => theme.gridUnit}px;
`;
+const RequiredFieldIndicator = () => (
+ <span
+ css={{
+ color: supersetTheme.colors.error.base,
+ fontSize: `${supersetTheme.typography.sizes.s}px`,
+ paddingLeft: '2px',
+ }}
+ >
+ *
+ </span>
+);
+
+const DescriptoinToolTip = ({ description }: { description: string }) => (
+ <ToolTipContainer>
+ <Tooltip
+ title={description}
+ placement="top"
+ overlayInnerStyle={{
+ display: '-webkit-box',
+ overflow: 'hidden',
+ // @ts-ignore -webkit-line-clamp is not in the CSS type
+ '-webkit-line-clamp': '20',
+ '-webkit-box-orient': 'vertical',
+ 'text-overflow': 'ellipsis',
+ }}
+ >
+ <Icons.InfoCircle
+ className="text-muted"
+ css={{ color: supersetTheme.colors.grayscale.light1 }}
Review comment:
Same thing here about using the theme directly.
##########
File path:
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx
##########
@@ -50,8 +52,59 @@ const StyledFilterControlContainer = styled(Form)`
width: 100%;
padding-right: ${({ theme }) => theme.gridUnit * 11}px;
}
+ .ant-form-item-tooltip {
+ margin-bottom: ${({ theme }) => theme.gridUnit}px;
+ }
+`;
+
+const FormItem = styled(StyledFormItem)`
+ .ant-form-item-label {
+ label.ant-form-item-required:not(.ant-form-item-required-mark-optional) {
+ &::after {
+ display: none;
+ }
+ }
+ }
`;
+const ToolTipContainer = styled.div`
+ font-size: ${({ theme }) => theme.typography.sizes.m}px;
+ display: flex;
+`;
+
+const RequiredFieldIndicator = () => (
+ <span
+ css={{
+ color: supersetTheme.colors.error.base,
+ fontSize: `${supersetTheme.typography.sizes.s}px`,
+ paddingLeft: `${supersetTheme.gridUnit}px`,
+ }}
+ >
+ *
+ </span>
+);
+
+const DescriptoinToolTip = ({ description }: { description: string }) => (
+ <ToolTipContainer>
+ <Tooltip
+ title={description}
+ placement="right"
+ overlayInnerStyle={{
+ display: '-webkit-box',
+ overflow: 'hidden',
+ WebkitLineClamp: 20,
+ WebkitBoxOrient: 'vertical',
+ textOverflow: 'ellipsis',
+ }}
+ >
+ <i
+ className="fa fa-info-circle text-muted"
+ css={{ paddingLeft: `${supersetTheme.gridUnit}px`, cursor: 'pointer' }}
Review comment:
Same here.
##########
File path:
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx
##########
@@ -50,8 +52,59 @@ const StyledFilterControlContainer = styled(Form)`
width: 100%;
padding-right: ${({ theme }) => theme.gridUnit * 11}px;
}
+ .ant-form-item-tooltip {
+ margin-bottom: ${({ theme }) => theme.gridUnit}px;
+ }
+`;
+
+const FormItem = styled(StyledFormItem)`
+ .ant-form-item-label {
+ label.ant-form-item-required:not(.ant-form-item-required-mark-optional) {
+ &::after {
+ display: none;
+ }
+ }
+ }
`;
+const ToolTipContainer = styled.div`
+ font-size: ${({ theme }) => theme.typography.sizes.m}px;
+ display: flex;
+`;
+
+const RequiredFieldIndicator = () => (
+ <span
+ css={{
+ color: supersetTheme.colors.error.base,
+ fontSize: `${supersetTheme.typography.sizes.s}px`,
+ paddingLeft: `${supersetTheme.gridUnit}px`,
Review comment:
One pixel here is better than 4 to make the required icon closer to the
text.
--
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]
