-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Aggregation picker changes for metrics #42053
Conversation
|
…1935-aggregation-picker
Codenotify: Notifying subscribers in CODENOTIFY files for diff 02e38e4...f822c62.
|
@@ -206,7 +206,7 @@ export const AccordionListCell = ({ | |||
</div> | |||
); | |||
} else if (type === "header-hidden") { | |||
content = <div className={CS.my1} />; | |||
content = <div className={cx({ [CS.my1]: itemIndex > 0 })} />; |
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.
Removes margin from the first empty header item
@@ -95,25 +94,24 @@ export function AggregationPicker({ | |||
const database = metadata.database(databaseId); | |||
const canUseExpressions = database?.hasFeature("expression-aggregations"); | |||
|
|||
if (operators.length > 0) { | |||
if (metrics.length > 0) { |
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.
reorders sections
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.
👍
Closes #41935
Figma https://www.figma.com/file/17Z8b1uEdcrtpP8rVHaPAS/Metrics-v2?type=design&node-id=1697-3003&mode=design&t=rwgYnuNJy2ynVE8G-0
Changes:
Please note that if you remove the metric aggregation clause in the notebook editor for a question that uses a metric we start to add
Count
by default in the summarize sidebar which looks strange/wrong. This is a separate issue .