Skip to content

Conversation

plexus77
Copy link

No description provided.

@neveldo
Copy link
Owner

neveldo commented Jan 20, 2016

Hello,

Thank for the PR !! getTextPosition() is called two times (one for the text initialization, where you have added you code, and another for the text update, that can occur when a user triggers an 'update' event on the container). Hence, I think that this feature should take place into the getTextPosition() in order to factorize the code.

Maybe, we could make the argument margin of the function getTextPosition() accept an integer (as currently) or an object such as {'x' : 5, 'y' : 10} that would allow us to fine tune the vertical and horizontal margin if needed.

If we receive an integer, we could transform the margin argument to an object :

if (typeof margin === "number") {
    margin = {x: margin, y: margin};
}

And then, we can deal with this object in the rest of the function to compute the proper text position.

What do you think about it ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be

if(options.text.attrs.dx !== undefined)

Note: if you are using typeof, you should compare to 'undefined' (notice the quote! typeof returns a string).
For Mapael, you can directly use undefined.

@Indigo744
Copy link
Collaborator

I agree with @neveldo. dx/dy should be set inside getTextPosition() function.

Regarding your second point, I think the margin name is not the best since we are not setting a real margin: we are adding a shift (or delta) to the position (x, y). Hence I think (dx, dy) fits nicely the description.
Sorry I misread your explanation. Indeed the existing margin option could be extended to accept a (x, y) object value to fine tune the margin.

@neveldo neveldo mentioned this pull request Mar 8, 2016
@plexus77
Copy link
Author

plexus77 commented Mar 8, 2016

Hi guys,
I only just saw this thread sorry, I would have refactored, let me know if you still need me to do anything.
Cheers :)

@neveldo
Copy link
Owner

neveldo commented Mar 9, 2016

Hello @plexus77 ,

Thank you, don't worry, we are working on it :) Here is a new PR : #204

@neveldo neveldo closed this Mar 9, 2016
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.

3 participants