sadpandajoe commented on code in PR #36349:
URL: https://github.com/apache/superset/pull/36349#discussion_r3685925556
##########
superset-frontend/src/explore/components/ControlHeader.tsx:
##########
@@ -95,15 +94,8 @@ const ControlHeader: FC<ControlHeaderProps> = ({
>
{description && (
<span>
- <Tooltip
- id="description-tooltip"
- title={description}
- placement="top"
- >
- <Icons.InfoCircleOutlined
- css={iconStyles}
- onClick={tooltipOnClick}
- />
+ <Tooltip title={description}>
+ <Icons.InfoCircleOutlined css={iconStyles} />
Review Comment:
This drops the `tooltipOnClick` handler, so the Time Pivot Frequency
control’s info bubble no longer opens the pandas offset-alias documentation
promised by its description. Could we keep forwarding this callback to the icon?
##########
superset-frontend/packages/superset-ui-core/src/components/Tooltip/index.tsx:
##########
@@ -16,22 +16,74 @@
* specific language governing permissions and limitations
* under the License.
*/
+import type { CSSProperties } from 'react';
import { forwardRef } from 'react';
import { Tooltip as AntdTooltip } from 'antd';
import type { TooltipRef } from 'antd/es/tooltip';
import type { TooltipProps, TooltipPlacement } from './types';
+import { resolveGlossaryString } from '@superset-ui/core';
+
+const TOOLTIP_SEPARATOR_STYLE: CSSProperties = {
+ margin: '8px 0',
+ border: 'none',
+ borderTop: '1px solid rgba(255, 255, 255, 0.2)',
+};
export const Tooltip = forwardRef<TooltipRef, TooltipProps>(
- ({ overlayStyle, ...props }, ref) => (
- <AntdTooltip
+ ({
+ overlayStyle,
+ title,
+ children,
+ ...props
+}, ref) => {
+ if (typeof title !== 'string') {
+ return (
+ <AntdTooltip
+ title={title}
+ styles={{
+ body: { overflow: 'hidden', textOverflow: 'ellipsis' },
+ root: overlayStyle ?? {},
+ }}
+ {...props}
+ >
+ {children}
+ </AntdTooltip>
+ );
+ }
+
+ const [glossaryUrl, description] = resolveGlossaryString(title);
Review Comment:
The existing Tooltip tests only exercise ordinary string titles, so none
would catch a regression in this new token-resolution branch. Could we add a
component case using `glossary.Query.Row_Limit.encode()` that asserts the docs
URL/target and the resolved description plus “Click to Learn More” after hover?
##########
superset-frontend/packages/superset-ui-core/src/glossary/glossary.ts:
##########
@@ -0,0 +1,116 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * Glossary definition containing terms organized by topic.
+ *
+ * ## How to add new glossary entries:
+ *
+ * 1. Add a new topic (if needed) or use an existing one
+ * 2. Add a term under the topic with a key (term name) and value object
containing:
+ * - short: A brief description (displayed in tooltips)
+ * - extended (optional): An extended description (displayed in
documentation)
+ *
+ * ## Example:
+ * export const glossaryDefinition: GlossaryDefinition = {
+ * Query: {
+ * Row_Limit: {
+ * short: t('Limits the number of rows...'),
+ * extended: t('Additional details...'), // optional
+ * },
+ * },
+ * };
+ *
+ * ## Formatting Notes:
+ * - Term names with underscores (e.g., `Row_Limit`) will be displayed with
spaces
+ * (e.g., "Row Limit") when rendered in the UI and documentation
+ */
+
+export const glossaryDefinition: GlossaryDefinition = {
+ Query: {
+ Dimension: {
+ short: t(
+ 'Dimensions contain qualitative values such as names, dates, or
geographical data. ' +
+ 'Use dimensions to categorize, segment, and reveal the details in
your data. ' +
+ 'Dimensions affect the level of detail in the view.',
+ ),
+ },
+ Metric: {
+ short: t(
+ 'Select one or many metrics to display. ' +
+ 'You can use an aggregation function on a column or write custom SQL
to create a metric.',
+ ),
+ },
+ Series: {
+ short: t(
+ 'Limits the number of series that get displayed. ' +
+ 'A joined subquery (or an extra phase where subqueries are not
supported) is applied ' +
+ 'to limit the number of series that get fetched and rendered. ' +
+ 'This feature is useful when grouping by high cardinality column(s)
' +
+ 'though does increase the query complexity and cost.',
+ ),
+ },
+ Row_Limit: {
+ short: t(
+ 'Limits the number of rows that get displayed. ' +
+ 'This feature is useful when grouping by high cardinality column(s)
' +
+ 'though does increase the query complexity and cost.',
+ ),
+ },
+ Sort: {
+ short: t(
+ 'Orders the query result that generates the source data for this
chart. ' +
+ 'If a series or row limit is reached, this determines what data are
truncated. ' +
+ 'If undefined, defaults to the first metric (where appropriate).',
+ ),
+ },
+ },
+ Advanced_Analytics: {
+ Time_Shift: {
+ short: t(
+ 'Overlay results from a relative time period. ' +
Review Comment:
This description is now also used by the legacy time-shift controls, but
those controls offer only fixed shifts and have neither “Inherit range from
time filters” nor “Custom”, so their tooltip directs users to options they
cannot select. Could the comparison-range-specific instructions remain separate?
--
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]
