🐛 Fix: Implement better snapping when dragging events from sidebar to grid #218
Conversation
…meday event to grid
not all tests are passing
There was a problem hiding this comment.
Nice job here! Snapping is tricky.
Functionality is good. Feels like a solid UX improvement.
I have a few requests.
Testing
The first is to add unit tests for the snapping logic. I added a test file and stubbed out a few uses cases to get it started. Feel free to adjust those as needed.
The reason I want to include tests is so we can more easily extend this snapping pattern for other use-cases. We could improve snapping for the timed week event, the all day row, and between sections of the sidebar. With that in mind, it'll be important to write these tests in a way that are
- Understandable: It should be relatively easy for a newcomer to read the tests, understand their intent, make changes, and write similar tests
- Focused: The view should be decoupled/abstracted from the test as much as possible. The tests should work whether we're snapping on a 7 day week, a 4 day week, or a day view. Minimal changes and setup are acceptable, but out tests and implementation shouldn't be tightly connected to our current view.
- Unit tests: Since our view is going to change, let's just focus on the logic. No need to write interaction or e2e tests for these. Those'll come in a separate effort.
Refactoring
You're right about how we shouldn't prop drill the measurements, refs, etc as much. Feel free to refactor as you go while you add tests. But take care not to go too far down this route. We can always do a separate refactor PR once we have the tests.
| remeasure: (elem: MeasureableElement) => { | ||
| console.log("remeasuring element", elem); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Passing around measurements leads to a lot of setup for these tests, which makes it harder to follow. I'm OK continuing this pattern for now, but it'd be good to do a follow-up sometime to extract this into a (Context) Provider, so that the test setup is more minimal
| Measurements_Grid, | ||
| } from "@web/views/Calendar/hooks/grid/useGridLayout"; | ||
|
|
||
| import { snapToGrid } from "./snap.grid"; |
There was a problem hiding this comment.
I recommend just testing snapToGrid, as that lets us effectively cover the X and Y coordinate logic without having to also test the snapYToGrid and snapXToGrid separately
| // Ideally we should fix how `mainGrid.left` is calculated to not include half the width of time column and | ||
| // include only the grid interactivity area (i.e. day columns). | ||
| // For now, we estimate the width of the time column to be 55px | ||
| const TIME_COLUMN_WIDTH = 55; |
There was a problem hiding this comment.
Agreed here. Something we should address after this PR is merged and we have more tests. No need to block PR for it now, though
Thank you! Noted. I'll make implementing tests my first priority. |
Description
Fixes issue
This PR handles implementing more accurate snapping when dragging someday events to the grid.
The issue the PR addresses happens due to a calculation mismatch between 2 methods:
GridEventPreview:getTimePreview()andGridEventPreview/styled:getItemStyles(). This is due to the methods relying on different calculation variables, producing different results in certain mouse movement edge cases.The PR fixes that by matching the calculation logic inside the
getItemStylesmethod with thegetTimePreviewmethod (it does not do it directly, because as a small refactor, we extracted snapping logic fromgetItemStylesinside its dedicatedsnapToGridfunction. SeesnapToGrid)To achieve that, we needed to feed calculation logic variable dependencies inside the tree, hence, why we needed to prop drill.
This is a common pattern inside the application and it is anti best practice. Ideally we should be using something like React context to feed variables down the tree without prop drilling.
I thought this type of refactor is out of scope of this PR, and I also felt like I was spending too much time on this PR so I wanted to tie things up and consult you from here.
As a side quest I snuck in an extra feature, horizontal snapping! (I took inspiration from google calendar and felt like we should implement it here too)
Results
Please take a look at the before and after, you will notice the following:
Before
vertical_snap_before.webm
After
vertical_snap_after.webm