Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: mouse cell selection with active editor #1382

Merged
merged 7 commits into from
Feb 12, 2024
Merged

fix: mouse cell selection with active editor #1382

merged 7 commits into from
Feb 12, 2024

Conversation

zewa666
Copy link
Contributor

@zewa666 zewa666 commented Feb 10, 2024

this fixes errors preventing to do a multi cell selection while an editor is opened.

Closes #1380

Copy link

codecov bot commented Feb 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bac08b0) 99.8% compared to head (fc104ad) 99.8%.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1382     +/-   ##
========================================
- Coverage    99.8%   99.8%   -0.0%     
========================================
  Files         199     199             
  Lines       21549   21557      +8     
  Branches     7200    7067    -133     
========================================
+ Hits        21486   21493      +7     
+ Misses         63      58      -5     
- Partials        0       6      +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -701,4 +705,52 @@ describe('CellRangeSelector Plugin', () => {
done();
}, 7);
});

it.only('should maintain propagation only if editor is on current cell', () => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops looks like you forgot to remove .only after you're done 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah thats super unfortunate ... I've added eslint-plugin-jest to detect those. Than started to fix the main ones by adjusting the the eslintignore file. As soon as I touched tests though, ~250 errors came up. So those should be handled in a separate shot not to mess up this PR

Copy link
Owner

@ghiscoding ghiscoding Feb 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm yeah that's just linting cosmetic though, it should indeed be better to add ESLint stuff as a separate PR. It looks like there's still a failing test though. It also looks like there's a line coverage missing too :)

@zewa666
Copy link
Contributor Author

zewa666 commented Feb 11, 2024

ok I think I got it set now. so what I've essentially changed is to apply previous behaviors only if the selected/iterated cell isn't the active editor cell. That means you still can't do a multi-select starting from the editor cell but it works for all others. I could have changed that as well with the original behavior from the patch I've mentioned, but this felt as the safer way, while fixing enough of the bug itself

@ghiscoding ghiscoding merged commit 17549b8 into ghiscoding:master Feb 12, 2024
4 checks passed
@ghiscoding
Copy link
Owner

ok I think I got it set now. so what I've essentially changed is to apply previous behaviors only if the selected/iterated cell isn't the active editor cell. That means you still can't do a multi-select starting from the editor cell but it works for all others.

that seems fine by me, I'm assuming most of the time it will be from another cell anyway. Thanks for the fix :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mouse multi cell selection does not work with active editor
2 participants