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

[Feature]: Updating width and height of an ellipse should update rx and ry and vice versa #10179

Open
4 tasks done
houssameddine-h opened this issue Sep 25, 2024 · 5 comments
Open
4 tasks done
Assignees
Labels

Comments

@houssameddine-h
Copy link

CheckList

  • I agree to follow this project's Code of Conduct
  • I have read and followed the Contributing Guide
  • I have searched and referenced existing issues, feature requests and discussions
  • I am filing a FEATURE request.

Description

Hi all,

The way ellipses are implemented for now is: if you update rx or ry the width and height get updated accordingly, but not the other way around (if you update the width or height the rx and ry don't get updated) which is more intuitive to actually do in my opinion.
The decision for this implementation was made here: #1062

This is a real problem, because when you try to resize an ellipse using the UI controls, it actually changes the width and height of the ellipse, which has no effect on the shape itself, but only updates the "container" of the shape.

It's easy enough to fix this in my own project, but I feel like everybody will have to do the same, since no one wants to resize a shape using the controls and having only the container changing.

The same behavior is implemented in circles, so we might fix that as well.

This is what you get when you try to resize an ellipse using the UI controls:

Screenshot 2024-09-25 at 23-33-37 React App

Current State

As a workaround for now, we can update these properties accordingly by listening to the relevant events.

Additional Context

No response

@asturur
Copy link
Member

asturur commented Sep 26, 2024

You can also just make some controls that update rx and ry instead of width and height.
The resize controls, the one that changes the width, was designed only for the textbox.
In general width/height drive changes in rect and textbox but are just a consequences of other properties in all other shapes.

@houssameddine-h
Copy link
Author

I see your point @asturur, but if those controls were designed specifically for textbox, then why not designing specific control behavior for other shapes as well?
I'm not sure of the amount of work that would take, but in the case of ellipses and circles it's not a lot.

I know that we should design the library to be generic, but since we already implemented different shapes, we should tweak the default controls to be specific to each shape.

@houssameddine-h
Copy link
Author

Also I'm not sure how to change the default behavior of the controls without having to redefine them all.

My initial approach was to listen for the object:scaling event and update the object properties.

@zhe-he
Copy link
Contributor

zhe-he commented Oct 9, 2024

I also have this confusion. I think the code should be as simple as possible. I hope the ellipse only has width and height, and that rx and ry are read-only. Wouldn't the following be more convenient?

class Ellipse2 extends FabricObject {
    
    get rx() {
        return this.width / 2;
    }
    get ry() {
        return this.height / 2;
    }
    
    // If backward compatibility is not a concern, the two methods below can be removed. 
    // If users want to set rx and ry, let them set the width and height instead.
    // set rx(n: number) {
    //    this.width = n * 2;
    // }
    // set ry(n: number) {
    //    this.height = n * 2;
    // }
}

@asturur
Copy link
Member

asturur commented Oct 14, 2024

i m against the 'viceversa' point.
There is a large confusion in fabricJS about width and height.

width and height as properties you can change works good for images and rect and nothing else.
On triangle works good only for triangles that have a flat base.

On text it make sense only to change width because height is a consequence

on polygon, paths and circle and ellipse are consequences of other properties r, rx, ry, points and commands.

In my opinion because of polygons and paths and text we have to move away from the width and height concept for most shapes. They need to exist internally as private properties and developers have to interact with the rx, r, ry, points and commands when trying to change a shape, while rect and image can continue to support width and height getters.

This is a major change tho

For what regards custom controls for ellipse and circle i can make a demo, it would help many people understand how to work with those. I ll do the one for ellipse that maybe is the most clarifying one

@asturur asturur added the docs label Oct 14, 2024
@asturur asturur self-assigned this Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants