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

Fp 1013 shift selecting multiple nodes and selecting multiple nodes by shift seeking the mouse are conflicting #374

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

quirinpa
Copy link
Contributor

@quirinpa quirinpa commented Nov 14, 2024

  • FP-1013: Shift selecting multiple nodes and selecting multiple nodes by shift seeking the mouse are conflicting

@quirinpa quirinpa self-assigned this Nov 14, 2024
@quirinpa quirinpa requested a review from a team as a code owner November 14, 2024 13:16
@quirinpa quirinpa force-pushed the FP-1013-shift-selecting-multiple-nodes-and-selecting-multiple-nodes-by-shift-seeking-the-mouse-are-conflicting branch 3 times, most recently from 8f61225 to fb1756a Compare November 14, 2024 13:43
@quirinpa quirinpa marked this pull request as draft November 14, 2024 13:52
@quirinpa quirinpa force-pushed the FP-1013-shift-selecting-multiple-nodes-and-selecting-multiple-nodes-by-shift-seeking-the-mouse-are-conflicting branch from fb1756a to 7e678b1 Compare November 14, 2024 14:56
@quirinpa quirinpa marked this pull request as ready for review November 14, 2024 14:57
Copy link
Contributor

@manuelnogueiramov manuelnogueiramov left a comment

Choose a reason for hiding this comment

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

I don't understand why so many changes. In my point of view, it should be something so simple as only do selection when the user start dragging, not as soon as shift is pressed.
This PR touches a lot of things that I'm not comfortable accepting

…lecting-multiple-nodes-by-shift-seeking-the-mouse-are-conflicting
@quirinpa
Copy link
Contributor Author

quirinpa commented Nov 22, 2024

Be sure to check what it does. I left those things there on purpose.
Check out what I'm doing.

Obviously what you had previously wasn't working.

However, this task might have been wrongly identified. I might have mixed it up with a task from @VicenteMovai .
Which was about drag multiple selection displacing too much.

However, I would like for you to review what I did with the flow. Please take a look at what's going on in MainInterface.
Using state like that is much easier than all that mode confusion. And it could potentially simplify the code a lot in the future.

@manuelnogueiramov
Copy link
Contributor

Be sure to check what it does. I left those things there on purpose.

I guess you didn't understand my point.
It's not about the solution. I bet it does exactly what was asked of you.
It's about the future regressions that will come out of this. My point is that you are making a lot of changes, for something that wouldn't be needing so many changes. That's just it. Which in turn might create future regressions.

@quirinpa
Copy link
Contributor Author

quirinpa commented Nov 25, 2024

I'm happy to cut some of the changes off, but I'd like it if you took a look at what happened in MainInterface.

Since we have talked about moving state to such an api class before and I would like you to understand what I mean.

I have understood your point. But you missed mine, again.

This is about what I told you about being able to persist react state outside the component without resorting to @tty-pt/sub or similar libraries. Just vanilla. This sort of thing would save us a lot of complexity. So please take a look at it. Even if we don't end up doing anything like it now, it is an important thing to consider.

I'll be happy to cut off some of the changes once you acknowledge what I'm trying to say here and take a look at what I mean.

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.

2 participants