chore(deps): update LG packages COMPASS-8611 #6569

mabaasit · 2024-12-16T09:43:52Z

Description

Checklist

New tests and/or benchmarks are included
Documentation is changed or added
I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

Bugfix
New feature
Dependency update
Misc

Open Questions

Dependents

Types of changes

Backport Needed
Patch (non-breaking change which fixes an issue)
Minor (non-breaking change which adds functionality)
Major (fix or feature that would cause existing functionality to change)

…pdate

addaleax · 2024-12-16T10:08:25Z

packages/compass-workspaces/src/index.spec.tsx

-      );
-    });
+    // TODO: The connection is disconnected somehow/somewhere which triggers cleanup and
+    // tabs are removed and these assertions fail. Need to figure this out!


Is this still outstanding?

phew, yeah! i spent quite some time in this PR to figure this out. i'll spend a bit more to figure out what triggers plugin cleanup for connections which leads to this failure.

Weirdly enough, doesn't look like it's failing in test-electron, so somehow this is runtime specific? It's very unexpected tbh, so we should really try to get to the bottom of this, ping me if you want to poke around in this together

i traced it down to mongoDBInstanceLocator not returning the correctly mocked instance. we hit this line and onNamespaceFallbackSelectRef is called and tab switches to default connection tab.

this actually also re-creates on main (if we give a small timeout before we do await waitFor).

after some time, i changed the strings to regex in 963497c (coll1 and coll3 to /coll1/i and /coll3/i) and it started to pass. i still think we should investigate it a bit more to see what's going on here. (i did not expect it to pass with this change)

mabaasit · 2024-12-16T10:10:47Z

packages/compass-workspaces/src/index.spec.tsx

+    // TODO: The connection is disconnected somehow/somewhere which triggers cleanup and
+    // tabs are removed and these assertions fail. Need to figure this out!


I have spent lots of time figuring this out, but did not find a solution yet.

mabaasit · 2024-12-16T10:11:33Z

packages/compass-components/src/hooks/use-confirmation.tsx

@@ -176,7 +175,6 @@ export const ConfirmationModalArea: React.FC = ({ children }) => {
          className: confirmationProps.hideCancelButton
            ? hideButtonStyles
            : undefined,
-          children: confirmationProps.cancelButtonText ?? 'Cancel',


Its always Cancel now and LG removed children prop.

Seems like it's still there, just directly using children now: https://github.com/mongodb/leafygreen-ui/blob/24c828d24c484cf8d02b777faa2dacc4b4ec2452/packages/confirmation-modal/src/ConfirmationModal/ConfirmationModal.tsx#L139

i think you are referring to confirmButton props, which still exists. but my comment refers to cancelButton props, which is defined as https://github.com/mongodb/leafygreen-ui/blob/24c828d24c484cf8d02b777faa2dacc4b4ec2452/packages/confirmation-modal/src/ConfirmationModal/ConfirmationModal.types.ts#L16-L17

Ah, right, misread when expanding the code in github, thanks for pointing this out!

mabaasit · 2024-12-16T10:23:58Z

packages/compass-components/src/hooks/use-stacked-component.tsx

-export const withStackedComponentPopoverStyles = function <
-  ComponentProps extends { popoverZIndex?: number }
->(
-  WrappedComponent: ComponentType<ComponentProps>
-): ComponentType<ComponentProps> {
-  return withBaseStyles(WrappedComponent, true);
-};


this becomes irrelevant with packages bump and Popover has marked popoverZIndex as deprecated.

Is there another way to provide the index now? We need to confirm this doesn't break compass-web in mms

Yes, i'll check this with mms integration (using sync-with-mms script)

...ons/src/components/aggregation-side-panel/stage-wizard-use-cases/group/group-with-subset.tsx

kraenhansen · 2024-12-16T15:03:01Z

packages/compass-aggregations/src/components/stage-toolbar/option-menu.tsx

+  const menuItems = useMemo(() => {
+    return [
+      {
+        label: 'Add stage after',
+        onClick: () => {
+          onAddStageClick(index);
+          setMenuOpen(false);
+        },
+        icon: 'PlusWithCircle',
+      },
+      {
+        label: 'Add stage before',
+        onClick: () => {
+          onAddStageClick(index - 1);
+          setMenuOpen(false);
+        },
+        icon: 'PlusWithCircle',
+      },
+      {
+        label: 'Delete stage',
+        onClick: () => {
+          onDeleteStageClick(index);
+          setMenuOpen(false);
+        },
+        icon: 'Trash',
+      },
+      {
+        label: 'Expand documents',
+        onClick: () => {
+          onExpand(index);
+          setMenuOpen(false);
+        },
+        icon: 'ChevronDown',
+      },
+      {
+        label: 'Collapse documents',
+        onClick: () => {
+          onCollapse(index);
+          setMenuOpen(false);
+        },
+        icon: 'ChevronUp',
+      },
+    ];
+  }, [index, onAddStageClick, onDeleteStageClick, onExpand, onCollapse]);
+


I've seen us doing this pattern elsewhere, but why do you see this being advantageous over simply rendering the children? I see possibilities of making the existing code more DRY without falling back on this pattern of building component trees from arrays of "data". In any case, would it make sense to separate this out into its own PR or does it have to be included in this work?

I added this to simply avoid adding data-text prop in the menu item because i did not want to repeat the text.
How would you prefer to clean this?

kraenhansen · 2024-12-16T15:07:30Z

packages/compass-components/src/components/document-list/element-actions.tsx

@@ -115,8 +115,6 @@ export const AddFieldActions: React.FunctionComponent<{
    <Menu
      open={isOpen}
      setOpen={setIsOpen}
-      usePortal={false}
-      popoverZIndex={999999}


kraenhansen

Always nice to see dependencies being updated 👍 I don't see anything holding back a merge.

mabaasit · 2024-12-16T17:31:10Z

Always nice to see dependencies being updated 👍 I don't see anything holding back a merge.

I am waiting to test this locally with mms integration and see if everything goes fine. My local setup it a bit broken and if it takes longer, I'll merge it.

mabaasit added 11 commits December 13, 2024 16:27

deps update

ba6e9f6

compile

06f18fd

migration

5fe77d8

remove renderMode where possible

f7cc742

fix stage operator select

d4986fa

fix tests

4238d83

fix tests

8619613

fix more tests

e22179c

remove menu item

771fd71

remove stacked popover component

9bb660d

Merge branch 'main' of https://github.com/mongodb-js/compass into lgu…

acc8c52

…pdate

addaleax reviewed Dec 16, 2024

View reviewed changes

mabaasit commented Dec 16, 2024

View reviewed changes

npm bootstrap

671f5d4

mabaasit commented Dec 16, 2024

View reviewed changes

mabaasit changed the title ~~chore(deps): update LG packages~~ chore(deps): update LG packages COMPASS-8611 Dec 16, 2024

addaleax reviewed Dec 16, 2024

View reviewed changes

...ons/src/components/aggregation-side-panel/stage-wizard-use-cases/group/group-with-subset.tsx Show resolved Hide resolved

kraenhansen reviewed Dec 16, 2024

View reviewed changes

kraenhansen approved these changes Dec 16, 2024

View reviewed changes

mabaasit added 3 commits December 16, 2024 16:39

fix label on input

a534b4d

use regex for test assertion

963497c

move to function call

444a1b7

Merge branch 'main' into lgupdate

4b20e81

addaleax approved these changes Dec 16, 2024

View reviewed changes

mabaasit merged commit 3c302b9 into main Dec 17, 2024
29 checks passed

mabaasit deleted the lgupdate branch December 17, 2024 09:02

kraenhansen mentioned this pull request Dec 17, 2024

chore(aggregations): Refactor aggregations stage toolbar #6572

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(deps): update LG packages COMPASS-8611 #6569

chore(deps): update LG packages COMPASS-8611 #6569

mabaasit commented Dec 16, 2024

addaleax Dec 16, 2024

mabaasit Dec 16, 2024 •

edited

Loading

gribnoysup Dec 16, 2024

mabaasit Dec 16, 2024

mabaasit Dec 16, 2024

mabaasit Dec 16, 2024

mabaasit Dec 16, 2024 •

edited

Loading

gribnoysup Dec 16, 2024 •

edited

Loading

mabaasit Dec 16, 2024

gribnoysup Dec 16, 2024

mabaasit Dec 16, 2024

gribnoysup Dec 16, 2024

mabaasit Dec 16, 2024

kraenhansen Dec 16, 2024 •

edited

Loading

mabaasit Dec 16, 2024

kraenhansen Dec 16, 2024

kraenhansen left a comment

mabaasit commented Dec 16, 2024

		// TODO: The connection is disconnected somehow/somewhere which triggers cleanup and
		// tabs are removed and these assertions fail. Need to figure this out!

chore(deps): update LG packages COMPASS-8611 #6569

chore(deps): update LG packages COMPASS-8611 #6569

Conversation

mabaasit commented Dec 16, 2024

Description

Checklist

Motivation and Context

Open Questions

Dependents

Types of changes

Choose a reason for hiding this comment

mabaasit Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mabaasit Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

gribnoysup Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen left a comment

Choose a reason for hiding this comment

mabaasit commented Dec 16, 2024

mabaasit Dec 16, 2024 •

edited

Loading

mabaasit Dec 16, 2024 •

edited

Loading

gribnoysup Dec 16, 2024 •

edited

Loading

kraenhansen Dec 16, 2024 •

edited

Loading