Skip to content

fix: Message Grouping card form improvements#1420

Merged
acco merged 3 commits intosequinstream:mainfrom
davoclavo:fix/group_messages_form_improvements
May 13, 2025
Merged

fix: Message Grouping card form improvements#1420
acco merged 3 commits intosequinstream:mainfrom
davoclavo:fix/group_messages_form_improvements

Conversation

@davoclavo
Copy link
Copy Markdown
Contributor

Improve Message Grouping card with the following changes:

  • Always show this card as an ExpandableCard, in order to show the Chevron and keep the layout consistent
  • For the Sink Edit form show tooltip with the notice "Message grouping can’t be changed for an active sink." on hover on the entire title bar
  • Change default expanded state whenever the card is shown in Sink Creation

Fixes #1383


Evidence

Sink Create form: expanded by default and no tooltip

image

Sink Edit form: show Tooltip on hover + show disabled Chevron

image

@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. ux-enhancement labels May 10, 2025
@davoclavo davoclavo changed the title Message Grouping card form improvements feat: Message Grouping card form improvements May 10, 2025
@davoclavo davoclavo changed the title feat: Message Grouping card form improvements fix: Message Grouping card form improvements May 10, 2025
@davoclavo davoclavo force-pushed the fix/group_messages_form_improvements branch from 2ede884 to d3a4ec3 Compare May 12, 2025 17:44
@davoclavo davoclavo requested a review from Copilot May 12, 2025 21:35
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves the Message Grouping card by standardizing its UI using an ExpandableCard component across both sink creation and sink edit modes. The changes include:

  • Enforcing a consistent ExpandableCard interface that shows a disabled state with a tooltip in edit mode,
  • Setting the card to be expanded by default in creation mode, and
  • Introducing a new ExpandableCardHeader component to encapsulate header behavior.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
assets/svelte/consumers/GroupColumnsForm.svelte Updates the Message Grouping card rendering to always use ExpandableCard with proper slots and properties for both edit and create modes.
assets/svelte/components/ui/card/expandable-card.svelte Modifies the card to conditionally wrap the header in a Tooltip when a titleTooltip is provided, ensuring consistent UI behavior.
assets/svelte/components/ui/card/expandable-card-header.svelte Introduces a new header component that manages state toggling and icon rotation in the ExpandableCard.
Comments suppressed due to low confidence (1)

assets/svelte/consumers/GroupColumnsForm.svelte:68

  • Verify that the disabled state does not inadvertently block tooltip trigger events on the card header. Ensure that users can still access the tooltip information on hover even when the card is disabled.
<ExpandableCard disabled={true} titleTooltip="Message grouping can’t be changed for an active sink.">

{disabled}
{#if titleTooltip}
<Tooltip.Root
openDelay={10}
Copy link

Copilot AI May 12, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider if the openDelay of 10 ms is sufficient for accessibility; a slightly longer delay might reduce accidental tooltip activations and improve usability.

Suggested change
openDelay={10}
openDelay={300}

Copilot uses AI. Check for mistakes.
Comment on lines +8 to +9
export let builders: $$Props["builders"] = [];

Copy link

Copilot AI May 12, 2025

Choose a reason for hiding this comment

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

[nitpick] Clarify and document the expected structure and type for the 'builders' prop to ensure future maintainability and readability.

Suggested change
export let builders: $$Props["builders"] = [];
/**
* An array of builder objects used to customize the behavior or appearance of the button.
* Each builder object should have the following structure:
* {
* id: string; // A unique identifier for the builder
* label: string; // A label describing the builder
* action: () => void; // A function to execute when the builder is triggered
* }
*/
export let builders: Builder[] = [];
// Define the Builder type
type Builder = {
id: string;
label: string;
action: () => void;
};

Copilot uses AI. Check for mistakes.
@acco
Copy link
Copy Markdown
Contributor

acco commented May 12, 2025

@davoclavo This looks good to me! One minor change: Regarding this:

Change default expanded state whenever the card is shown in Sink Creation

We actually purposefully show it as collapsed on create, as users rarely change/deviate from the default on this one. That may change in the future. (We think 80%+ will want us to group by primary key)

Note if the table does not have any primary keys, then I believe it will be expanded –– as there is no default behavior. Might be worth checking what the behavior is there!

davoclavo added 3 commits May 12, 2025 17:47
- The default state for tables with no primary keys is to expand the card, and show the option to select which column to group on. And disabled the collapsing behavior from the card as a column *must* be selected.
- Made a change on ExpandableCards to allow this extra customization of disabling a card in either collapsed or expanded state. Previously a disabled card forced the state to be collapsed.
- Added `type="button"` to the `ExpandableCardHeader` otherwise was causing some UX issues like submitting the form or showing form validation results unexpectedly.
@davoclavo davoclavo force-pushed the fix/group_messages_form_improvements branch from d3a4ec3 to 6037005 Compare May 13, 2025 00:36
@davoclavo
Copy link
Copy Markdown
Contributor Author

@acco Thanks for the feedback!

I just pushed the changes for the behavior you pointed out, and also identified a couple of extra improvements.

  • Message grouping card
    • In Sink Create mode
      • For tables with primary keys the card is shown as collapsed by default, as you suggested, and expanding is available.
      • For tables without primary keys the card is shown as expanded by default, in order to show the option to select which column to group on. Also disabled the collapsing behavior from the card as a column must be selected.
    • In Sink Edit mode
      • Card is always collapsed and disabled. Also shows the tooltip stating that grouping can only be done when a sink is created
  • Made a change on ExpandableCards to allow an extra customization of disabling a card in either collapsed or expanded state. Previously a disabled card forced the state to be collapsed.
  • Added type="button" to the ExpandableCardHeader component, otherwise was causing some UX issues like submitting the form or showing form validation results unexpectedly.

Evidence

Sink Create - using table with primary keys

image

Sink Create - using table without primary keys

image

Sink Edit - using table with primary keys

image

Sink Edit - using table without primary keys

image

@acco
Copy link
Copy Markdown
Contributor

acco commented May 13, 2025

@davoclavo Awesome work, all sounds good!

@acco acco merged commit a7842cf into sequinstream:main May 13, 2025
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files. ux-enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clearly show "Message grouping" as disabled in edit state

3 participants