refactor: table connections standardized and single by default. Special case for dashboard #1195
Conversation
case for dashboard
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
E2E Test Results✅ All tests passed • 21 passed • 3 skipped • 150s
|
| const { result } = renderHook( | ||
| () => | ||
| useGetKeyValues({ | ||
| chartConfigs: mockChartConfigs, | ||
| keys: mockKeys, | ||
| }), | ||
| { wrapper }, | ||
| ); |
There was a problem hiding this comment.
Could this be updated to be a test for useMultipleGetKeyValues instead of removing it? Or is there another reason to remove it?
There was a problem hiding this comment.
Good call! I initially took this PR in a different direction which is why I deleted it. Going to add it back in with that new hook
| tableConnection?: TableConnection; | ||
| tableConnections?: TableConnection[]; |
There was a problem hiding this comment.
Overall I think the refactor is great, but this type update seems like a step backwards since the requirement that we provide either a list or a single object is no longer baked into the type. Consumers could provide both and typescript wouldn't complain. Thoughts on updating this type to require that only one is provided, or reverting it back to accepting a list or a single object and using the isArray check you use elsewhere to turn the object into a list if necessary?
Same question for SearchInputV2
There was a problem hiding this comment.
Good idea! I like the explicitness you're outlining that typescript should complain if both are provided. I updated with a new union type that will cause a typescript error if both are provided
Closes HDX-2469