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

[Bug]: Align guidless changes aspect ratio on snapping when scaling #10114

Open
7 tasks done
hamzawain7 opened this issue Aug 31, 2024 · 6 comments
Open
7 tasks done

[Bug]: Align guidless changes aspect ratio on snapping when scaling #10114

hamzawain7 opened this issue Aug 31, 2024 · 6 comments

Comments

@hamzawain7
Copy link

CheckList

  • I agree to follow this project's Code of Conduct
  • I have read and followed the Contributing Guide
  • I have read and followed the Issue Tracker Guide
  • I have searched and referenced existing issues and discussions
  • I am filing a BUG report.
  • I have managed to reproduce the bug after upgrading to the latest version
  • I have created an accurate and minimal reproduction

Version

6.0.2

In What environments are you experiencing the problem?

Chrome

Node Version (if applicable)

None

Link To Reproduction

See attached recording

Steps To Reproduce

ScreenRecording2024-08-31at1 46 09PM-ezgif com-video-to-gif-converter

@zhe-he I love the new align guidelines, but after I updated production with these new guidelines, users pointed out that it ruins the aspect ratio of the items. It's quite bad if there are a lot of items in the design.

An additional feature would be to expose the snapping value in the aligningLineConfig. Leme know how can i help with it if you are busy. May i look at the code and create a pull request myself?

Expected Behavior

Both scaleX and scaleY should change so that the aspect ratio remains the same.

Actual Behavior

Only scaleX or scaleY changes

Error Message & Stack Trace

No response

@asturur
Copy link
Member

asturur commented Aug 31, 2024

Yes you can look at the code and make a PR.
I didn't have time to compare old and new implementation.
This is a new feature, unplanned, and we have to think is probably broken ( no offense @zhe-he, thank you for starting this ).
There are NO TESTS apart the ones i quickly added.
Being provided with no tests i also have to understand what the code is supposed to do by reverse engineering or by asking.

Fixing this feature will be hard and tests are required for changes, sorry if that put some extra weight on the contributor but at this point are kind of necessary

@asturur
Copy link
Member

asturur commented Aug 31, 2024

There are also som extra comments to do here.
Snapping is something that goes both on controls and dragging, while the guidelines are something that are on render.
I would have preferred to move those as controls for helpers while right now i think they are all implemented on events that is kind of unfortunate.

@zhe-he
Copy link
Contributor

zhe-he commented Sep 2, 2024

@hamzawain7
I'll think about how to fix it, and I'll address this issue later

@zhe-he
Copy link
Contributor

zhe-he commented Sep 2, 2024

@asturur
I have solved this problem, but I don't know how to write test cases. Can you help me write them? Or can you tell me where I can find example demos? Thanks.

#10120

@zhe-he
Copy link
Contributor

zhe-he commented Sep 2, 2024

@hamzawain7
I use canvas.uniScaleKey and canvas.uniformScaling to determine if you are scaling simultaneously. If you have custom controllers but the scaling logic differs from the convention, please ensure it is consistent with the convention.

@asturur
Copy link
Member

asturur commented Sep 2, 2024

Test cases for interactions are hard indeed. Let's review the PR first we can add it there

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

No branches or pull requests

3 participants