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

Proposal for different lineJustify. #4437

Merged
merged 5 commits into from
Nov 12, 2017
Merged

Proposal for different lineJustify. #4437

merged 5 commits into from
Nov 12, 2017

Conversation

asturur
Copy link
Member

@asturur asturur commented Nov 4, 2017

@jilster @blobinabottle
replaces #4369

This is my idea to implement it, giving us 3 different options.

To be honest i fast draft it, i did not event tested it.

@@ -119,7 +119,7 @@
textLeftOffset += charBox.kernedWidth - charBox.width;
}
boxWidth += charBox.kernedWidth;
if (this.textAlign === 'justify' && !timeToRender) {
if (this.textAlign.indexOf('justify') !== -1 && !timeToRender) {
Copy link
Member Author

Choose a reason for hiding this comment

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

this was not replaced in the whole code in the other PR,

@@ -348,6 +348,9 @@
enlargeSpaces: function() {
var diffSpace, currentLineWidth, numberOfSpaces, accumulatedSpace, line, charBound, spaces;
for (var i = 0, len = this._textLines.length; i < len; i++) {
if (i === len - 1 || this.isEndOfWrapping(i)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

ok here needs a this.textAlign !== 'justify' && ( ..... )

Copy link
Member Author

Choose a reason for hiding this comment

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

i still think this change is needed.
Since we are giving the user

justify-left, jsutify-right, justify-center, this thing here should change that we do not want the old plain justify.

Who built an app over 2.0b7 or earlier, does not want to see old JSON saved with plain justify loaded differently just because we think is better to have last line not justified.

In this way we give an option to everyone at expense of littled added complexity, and we do not break old stuff.

Copy link

@jilster jilster Nov 8, 2017

Choose a reason for hiding this comment

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

Sorry for the late response. Yes, i agree, backwards compatibility is important. Will you change it or should i?

@asturur
Copy link
Member Author

asturur commented Nov 4, 2017

@jilster since you started the whole thing, i would be happy if yo accelerate the thing you could test this branch and open a PR over it in order fix what is not really working.
If you have time.

@jilster
Copy link

jilster commented Nov 5, 2017

Thanks, i will do that!

@jilster
Copy link

jilster commented Nov 6, 2017

See #4441

@jilster
Copy link

jilster commented Nov 12, 2017

i don't know if you were notified when i replied on a code review comment, but just to be sure: i am happy to do some last fixes if needed.

@asturur
Copy link
Member Author

asturur commented Nov 12, 2017

i spent the weekend on fabricjs basically.
i cleared the top things, i want to try this now to be sure is working as expected/intended by me.
I also try to add some tests if possible.

@asturur
Copy link
Member Author

asturur commented Nov 12, 2017

image

example textselection we forgot it.

@asturur
Copy link
Member Author

asturur commented Nov 12, 2017

image

text decoration not working at all...

@asturur asturur merged commit 10a8684 into master Nov 12, 2017
@jilster
Copy link

jilster commented Nov 13, 2017

very nice! 👍

@blobinabottle
Copy link
Contributor

Thank you guys! great job!

@asturur asturur deleted the test-justify branch November 19, 2017 15:31
@asturur asturur mentioned this pull request Nov 19, 2017
thiagocunha pushed a commit to thiagocunha/fabric.js that referenced this pull request Nov 18, 2019
* not even tested

* Small fixes (fabricjs#4441)

* fixed selection

* fixed svg export
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