feat: improve Timepoint Explorer selection UX#1574
Conversation
Script size changes
Totals
|
| display: flex; | ||
| flex-direction: column; | ||
| gap: ${theme.spacing(2)}; | ||
| scroll-margin-top: 25vh; |
There was a problem hiding this comment.
This scrolls to 25% of the view height above the logs, so that the user will be able to see a little bit of the graph
| from: dateTime('2024-01-01T00:00:00Z'), | ||
| to: dateTime('2024-01-02T00:00:00Z'), |
There was a problem hiding this comment.
The XAxis component is expecting a DateTime object. It didn't throw an error before because we are not mocking timepoints which now renders the XAxisContent
ckbedwell
left a comment
ckbedwell
left a comment
There was a problem hiding this comment.
Great changes. 👏
Main CR is just exploring if MSW would be better rather than mocking the hook.
| jest.mock('scenes/components/TimepointExplorer/TimepointExplorer.hooks', () => ({ | ||
| ...jest.requireActual('scenes/components/TimepointExplorer/TimepointExplorer.hooks'), | ||
| useTimepoints: jest.fn(() => mockTimepoints), | ||
| })); | ||
|
|
||
| jest.mock('scenes/components/TimepointExplorer/TimepointViewer.hooks', () => ({ | ||
| useTimepointLogs: jest.fn(() => ({ | ||
| data: [], | ||
| isFetching: false, | ||
| isLoading: false, | ||
| refetch: jest.fn(), | ||
| })), | ||
| })); |
There was a problem hiding this comment.
I'm not necessarily against mocking the hook here but my preference is we use MSW to mock the base dependency. The hook is an implementation detail so if we were to change it at some point in the future we would break the tests, which undermines one of their main benefits. If we aim to mock as high as possible (our out-of-repo dependencies) and test from the perspective of a user we create the most durable tests.
So, try and mock the server response instead here and see how that looks/feels. The Time Point Explorer does have a lot of inter-connected dependencies which is why it doesn't have many tests so we might find this approach is a pain in the ass as it creates a horrible web and what you've done here is the cleanest solution but give it a go and see how you find it!
| const mockScrollIntoView = jest.fn(); | ||
| Element.prototype.scrollIntoView = mockScrollIntoView; |
There was a problem hiding this comment.
Can this get reset and/or moved out of the global scope if possible?
| timepoints: StatelessTimepoint[]; | ||
| } | ||
|
|
||
| export function useIsInitialised({ |
There was a problem hiding this comment.
How confident are we we don't need this?
There was a problem hiding this comment.
I'll put it back. I initially had to remove it because the initialization selects a timepoint, so it would auto-scroll on page-load. I then added the shouldScrollToViewer flag that only enables on timepoint click, so that issue won't be the case anymore
g3john
force-pushed
the
feat/improve-timepoint-explorer-selection
branch
from
b551dca to
57f7c41
Compare
ckbedwell
left a comment
ckbedwell
left a comment
There was a problem hiding this comment.
Was just about to approve this then noticed there is one place you've missed that doesn't scroll to the logs viewer (it is fairly hidden 😅)
Screen.Recording.2026-02-03.at.17.16.39.mov
Once that is done, this is going in!
Ah thanks for calling that one out. Added! |
2 participants
What this PR does / why we need it :
This PR makes the following changes
Which issue(s) this PR fixes:
This PR addresses the first
Interaction patterns to improve responsiveness & discoverabilityitem in this docScreen.Recording.2026-01-30.at.12.03.44.PM.mov