feat(sidebar): active connections toggle COMPASS-8114 #6458
Conversation
Use default align for the Tooltip
kraenhansen
force-pushed
the
kh/active-connections-toggle
branch
from
40b2188 to
dc649bf
Compare
kraenhansen
changed the title
Use queryByTestId to fix test
Move asserts into a shared expectElementByTestId
| function expectElementByTestId(testId: string, state: 'visible' | 'missing') { | ||
| if (state === 'visible') { | ||
| expect(screen.queryByTestId(testId)).to.be.visible; | ||
| } else if (state === 'missing') { | ||
| expect(screen.queryByTestId(testId)).to.be.null; | ||
| } | ||
| } |
There was a problem hiding this comment.
If we want a generalized helper I think it would make a bit more sense so add this as an extra chai assertion instead of a function here, it's more effort but way more consistent with how we assert, which is always done with chai library asserts
There was a problem hiding this comment.
I did experiment a bit with it. The question is what we'd want that expect to take as an argument.
If it takes the test-id string attribute, I don't know what to call that chai assert property 🙈 expect("some-test-id").to.be.testIdOfElementInDocument?
Thinking more about it, perhaps it's overkill with this helper 🤔 I mostly wanted to find a way to codify the pattern.
There was a problem hiding this comment.
I've reverted the helper and cleaned up the test instead: 4450dc7 - I think that's better 🤞
There was a problem hiding this comment.
I think it's a good idea, but I'd still have a strong preference to stick to the interface of chai asserts we are accustomed to using if we want to pursue this. And yeah, just a test-id is probably not a great argument for chai here. Maybe for case that you were running in something like expect(elementOrNull).to.be.in.dom that checks that the element is not null and window.document.contains is true can be a new interface for what you're trying to check here. Then you'd be able to combine then with visible check too
Revert the shared helper
Remove unused import
| @@ -167,6 +169,38 @@ describe('useFilteredConnections', function () { | |||
| }); | |||
| }); | |||
|
|
|||
| context('excluding inactive connections', function () { | |||
| it('should match only connected collections items', function () { | |||
There was a problem hiding this comment.
| it('should match only connected collections items', function () { | |
| it('should match only connected connection items', function () { |
typo I guess?
| // Conditionally skip connections that aren't considered active | ||
| const inactive = | ||
| connection.connectionStatus !== 'connected' && | ||
| connection.connectionStatus !== 'connecting'; |
There was a problem hiding this comment.
I don't know if there was a decision taken on this so confirming - are we sure we want to count connecting connections as active?
|
I talked with @bsradcliffe on Slack and he has suggestions to redesigning the flow / experience. |
|
I just filed COMPASS-8503 for the followup work. We should make sure to update the designs before the next GA Compass release or revert this PR; it could be confusing for users to have this UX update come shortly after adding this toggle. |
Description
Merging this PR will:
IconButtonto the right of theNavigationItemsFilterin theConnectionsNavigation.Sidebarcomponent.useFilteredConnectionshook.active-connections-toggle.mov
Checklist
Motivation and Context
Open Questions
IconButtonin theNavigationItemsFiltercomponent instead and update theonFilterChangeto take an object with theRegExpas well as aboolean.Dependents
Types of changes