geido commented on code in PR #33256:
URL: https://github.com/apache/superset/pull/33256#discussion_r2063970219
##########
superset-frontend/packages/superset-ui-chart-controls/src/components/InfoTooltipWithTrigger.tsx:
##########
@@ -16,56 +16,60 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { CSSProperties } from 'react';
+import { CSSProperties, KeyboardEvent, ReactNode } from 'react';
import { kebabCase } from 'lodash';
import { t } from '@superset-ui/core';
+import { ExclamationCircleOutlined } from '@ant-design/icons';
import { Tooltip, TooltipProps, TooltipPlacement } from './Tooltip';
export interface InfoTooltipWithTriggerProps {
label?: string;
tooltip?: TooltipProps['title'];
- icon?: string;
+ icon?: ReactNode;
onClick?: () => void;
placement?: TooltipPlacement;
- bsStyle?: string;
className?: string;
iconsStyle?: CSSProperties;
}
-export function InfoTooltipWithTrigger({
+export const InfoTooltipWithTrigger = ({
label,
tooltip,
- bsStyle,
onClick,
- icon = 'info-circle',
+ icon = <ExclamationCircleOutlined />,
className = 'text-muted',
placement = 'right',
- iconsStyle = {},
-}: InfoTooltipWithTriggerProps) {
- const iconClass = `fa fa-${icon} ${className} ${
- bsStyle ? `text-${bsStyle}` : ''
- }`;
+ iconsStyle = { fontSize: 12 },
Review Comment:
I don't think we need this prop at all. The `fontSize` appear to be always
the same? Also, if we really need it, it would make more sense to apply the
styles to the `icon` and not indirectly through the props.
##########
superset-frontend/src/explore/components/controls/AnnotationLayerControl/index.tsx:
##########
@@ -193,7 +193,7 @@ class AnnotationLayerControl extends PureComponent<Props,
PopoverState> {
return (
<InfoTooltipWithTrigger
label="validation-errors"
- bsStyle="danger"
+ iconsStyle={{ color: theme.colorError }}
Review Comment:
I think the previous approach of standardizing the component via "danger",
"warning", etc. was better than this. We should rethink the approach here. Just
standardize the possible statuses and use the appropriate Icon directly within
the component, otherwise this just becomes a wrapper for icons and it does not
make a lot of sense.
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/Option.tsx:
##########
@@ -72,9 +72,13 @@ export default function Option({
<Label data-test="control-label">{children}</Label>
{(!!datasourceWarningMessage || isExtra) && (
<StyledInfoTooltipWithTrigger
- icon="exclamation-triangle"
+ icon={
Review Comment:
Same comment as above. Let's standardize with "danger", "warning" etc. If we
do not need to customize the icon, then we can remove the prop entirely and
just standardize the component.
--
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]
