Skip to content
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

Add UI to swap clauses in the notebook #39205

Merged
merged 48 commits into from
Mar 11, 2024
Merged

Conversation

ranquild
Copy link
Contributor

@ranquild ranquild commented Feb 27, 2024

Epic #39210
Demo https://www.loom.com/share/46322c56db044c4d8d8de5c890510e62

Allows to drag-n-drop custom columns, filters, aggregations, and breakouts.

How to verify:

  • Add a question with all of these clauses
  • Try to reorder them via drag-n-drop

@ranquild ranquild changed the base branch from master to 38862-legacy-popover February 27, 2024 14:27
@@ -65,6 +65,8 @@ export type OrderByDirection = "asc" | "desc";
declare const FilterClause: unique symbol;
export type FilterClause = unknown & { _opaque: typeof FilterClause };

export type Filterable = FilterClause | ExpressionClause | SegmentMetadata;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to Aggregatable

@ranquild ranquild added the no-backport Do not backport this PR to any branch label Feb 29, 2024
@ranquild ranquild changed the base branch from master to mblib-swap-clauses March 8, 2024 14:19
@ranquild ranquild requested a review from a team March 8, 2024 15:41
@ranquild ranquild changed the title [WIP] Add UI to swap clauses in the notebook Add UI to swap clauses in the notebook Mar 8, 2024
@ranquild ranquild marked this pull request as ready for review March 8, 2024 15:42
@ranquild ranquild requested a review from cdeweyx March 8, 2024 15:42
const { attributes, listeners, setNodeRef, transform, transition } =
useSortable({
id,
animateLayoutChanges: () => false,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disable animation after drag-n-drop because we don't have a stable ID

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment deserves to live in code

);
}

function getItemIdFromIndex(index: number) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DnD kit doesn't like 0 as id so we convert indexes to strings

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment deserves to live in code

Copy link

replay-io bot commented Mar 8, 2024

Status Complete ↗︎
Commit b821015
Results
1 Failed
⚠️ 11 Flaky
2321 Passed

Copy link
Contributor

@kamilmielnik kamilmielnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great 👍

Comment on lines +85 to +90
export function swapClauses(
query: Query,
stageIndex: number,
sourceClause: Clause,
targetClause: Clause,
): Query {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing that every *Step component has its own handleReorder* implementation, I thought that the benefit is type safety - it should not be possible to swap an aggregation clause with a breakout clause, etc.

We could enforce it here:

Suggested change
export function swapClauses(
query: Query,
stageIndex: number,
sourceClause: Clause,
targetClause: Clause,
): Query {
export function swapClauses<
C extends
| AggregationClause
| ExpressionClause
| BreakoutClause
| FilterClause
| OrderByClause,
>(query: Query, stageIndex: number, sourceClause: C, targetClause: C): Query {

["sum", ["field", ORDERS.SUBTOTAL, null]],
["sum", ["field", ORDERS.TOTAL, null]],
["avg", ["field", ORDERS.TOTAL, null]],
],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also cover order clauses

const { attributes, listeners, setNodeRef, transform, transition } =
useSortable({
id,
animateLayoutChanges: () => false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment deserves to live in code

);
}

function getItemIdFromIndex(index: number) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment deserves to live in code

@kamilmielnik kamilmielnik requested a review from a team March 11, 2024 08:06
@ranquild ranquild merged commit 61f7bdd into mblib-swap-clauses Mar 11, 2024
75 checks passed
@ranquild ranquild deleted the 38862-clause-dnd branch March 11, 2024 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/QueryingComponents
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants