chore(compass-components): update LeafyGreen packages, update table usage COMPASS-7046

[Anemy]

COMPASS-7046

This pr updates our LeafyGreen dependencies to latest. The bulk of the changes are in the new table adoption, mostly for indexes.

The only visual changes here are in the Indexes tab where we've removed the card. The new table has a few smaller visual changes too.

before	after

[gribnoysup]

Do we still need to maintain our fork?

[Anemy]

This is a good question, I'm not sure currently. If it's alright with you I'd like to do that (if an investigation shows we can remove it) as a follow up pr. I don't think it impacts these changes so it should be something we can handle independently.

[gribnoysup]

Yeah, I think it's fine to keep it for the update in this PR, but would be really really nice to figure out if we can finally remove it, not the best thing to maintain on our own in the repo 😅

[Anemy]

Going to freeze this for the time being. The main remaining issue is a matter of fixing or working around how tabbable interacts with elements in jsdom in our tests so they pass, then we're good to merge once we fix any leftover e2e tests that might need updated selectors.

The issue with tabbable is that when we upgrade our LeafyGreen dependencies we get a new version of focus-trap-react and focus-trap which use a new version of tabbable. This newer version of tabbable fails to tab to a focusable element in our mocha tests because of lack of jsdom support. The line that errors is here: https://github.com/focus-trap/focus-trap/blob/0881d9ea9a087fd3ecc97f32e406926c69aaabaf/index.js#L397

The issue is described in tabbable's README: https://github.com/focus-trap/tabbable?tab=readme-ov-file#testing-in-jsdom

There are some workarounds we could do, we could:
Skip our GuideQueue tests and FeedbackPopover test that use the focus trap and rely on e2e tests to cover them. Seems like the easiest and most viable.
Somehow fix it upstream and then wait for that to be released and update it in our dependencies and LeafyGreen (would be cool but probably impractical). There are a number of GitHub issues that mention this issue like focus-trap/tabbable#664
Two we probably don't want to do:
Do something similar to the global overwrite suggested in the tabbable README link above for our unit test environments. I'd like to avoid this if possible, it's hacky.
Override the tabbable dependency version to an older one with something like overrides although I don't know how to do this with our lerna package configuration. Also hacky and could lead to issues down the line with dependency management.

Maybe I'm missing something and there's some other fix we can do.

Some other notes:

Looks like the LeafyGreen team also ran into this which caused them to downgrade their focus-trap in the GuideQueue:
https://github.com/mongodb/leafygreen-ui/blob/main/packages/guide-cue/CHANGELOG.md#major-changes

We already workaround this issue in our own FocusTrap usage here:

compass/packages/compass-components/src/components/interactive-popover.tsx

Line 186 in 2a270bd

// Tests fail without a fallback. (https://github.com/focus-trap/focus-trap-react/issues/91)

Interestingly these errors also starts happening locally when I upgrade to node 20+. I'm thinking maybe because something is resolved differently in our dependencies which brings in the newer tabbable version but I'm not sure. So this may become a blocker down the line in more than one way if we don't fix it now.

[gribnoysup]

The only e2e test issue left is that selectbox / combobox is now again unusable in CI for whatever reason, investigating...

[alenakhineika]

Question, in which cases do we want to set this as prop? How is it different from just using <H3></H3>?

[alenakhineika]

Why we don't await here anymore?

[lerouxb]

browser.$() just wraps the selector into an object you can then chain. so things that actually check things or perform some action on that element only really need the await. But we're not consistent about it at all. See below in this same file where he does const button = await browser.$( 😄

The documentation uses await, so that's probably better: https://webdriver.io/docs/api/browser/$

[gribnoysup]

From the docs:

Note: only use these element objects if you are certain they still exist on the page, e.g. using the isExisting command. WebdriverIO is unable to refetch them given that there are no selector information available.

This script does a bunch of async steps, some of which are happening in a loop with waitUntil, I'd prefer webdriver to always fetch the element before running operations on it instead of failing unexpectedly with "element doesn't exist". This was the flaky part of the test, so I want to make sure it's more stable than before. Other parts of the script here didn't fail, so they are not changed. I don't want to pull more unneccessary changes to this PR. Would you be okay with having a ticket to refactor this later?

[lerouxb]

I'm not actually sure if you do:

const menuChainablesomething = browser.$(menuSelector)
const menuElement = await menuChainablesomething;

Is the element now cached on menuChainablesomething or only on menuElement? Just thinking out load. This kind of ambiguity (menuChainablesomething is both a promise and.. something?) is why I usually try and not keep that object around. But I don't think I've been very consistent about it. We definitely cache element objects in various places where we shouldn't. I've tried to reuse a selector before or wrap getting the menuElement (or that other intermediate promise-like object) in a get function. Maybe not necessary.

[lerouxb]

side-note: I don't find their documentation very clear here. There are two types in play: ChainablePromiseElement<WebdriverIO.Element> and WebdriverIO.Element and I think it could be clearer.

[gribnoysup]

Just to make sure the context didn't get lost in private messages, I did a test and indeed stuff changed over time and the chainable caches the values after initial call. The wdio documentation still suggests to not await on element before using commands, but for different reasons: mostly just due to this being unneccessary