-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Resize vertical lanes #2067
Resize vertical lanes #2067
Conversation
23ae9e6
to
886ca61
Compare
Hi, thanks for this PR! I am looking into this today.
I pushed line endings change before your commits to make the review easier. Other than that, I believe it's OK. |
886ca61
to
9537d0b
Compare
I'm sorry, I didn't manage to have a thorough review of this, but will have a look next week. |
@barmac Don't worry. Thanks for the time you invest in the review. I used the chance to make an adjustment. Unfortunately, I created multiple commits. Please provide some squashing support when checking that in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once again sorry for the delay. The interaction is mostly good, but there is a difference in the minimal height when shrinking happens from the bottom and from the top:
Screen.Recording.2024-01-19.at.16.50.16.mov
BTW I've found an edge case which can be reproduced with horizontal participants as well. There is no need to fix this in this PR though. I wouldn't really bother creating an issue for this, too: Screen.Recording.2024-01-19.at.16.53.10.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A challenge to solve in the future:
Screen.Recording.2024-01-19.at.17.06.27.mov
This problem can be noticed only if the participant contains child lanes. |
@barmac Thank you very much for your thorough review. Glad you were able to make it.
I can reproduce this also for horizontal pools. I'd be happy if we could treat this as a pre-existing bug and not as a failure in the PR, just like the second issue you discovered.
This is actually on my list already, but out of scope for this PR. I plan to fix add/delete/split separately. |
I was able to reproduce it for horizontal lanes too. Let's keep as is it then. |
Closes #2062
This makes resizing by handles and space tool work for vertical pools and lanes.
I just saw that ResizeBehavior.js, SpaceToolBehavior.js and their Spec files have Windows line endings that my IDE didn't make me aware of. They now have Unix-style line endings. Is that acceptable for you?