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

feat(text.class.js) add path rendering to textpaths #7328

Merged
merged 9 commits into from
Aug 27, 2021

Conversation

melchiar
Copy link
Member

This adds path rendering for text objects that contain a path.

image

@asturur - let me know if you're okay with this implementation and I'll add a visual test.

@@ -498,6 +498,7 @@
* @param {CanvasRenderingContext2D} ctx Context to render on
*/
_render: function(ctx) {
this.path && this.path._render(ctx);
Copy link
Member

Choose a reason for hiding this comment

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

mmmm
let me understand this.
So if i don't want to draw a path, what do i do?
i have to set strokeWidth or stroke empty or fill empty?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's my thinking yes. Currently though there's no check for the visible property, so maybe I should add that as one more way to hide the path?

Copy link
Member

Choose a reason for hiding this comment

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

good idea. visible is checked somewhere else higher up in the rendering chain, but yes adding that here is ok. and opacity > 0.

@asturur
Copy link
Member

asturur commented Aug 25, 2021

i'm not opposed to this, it sounds strange initially, but if i stop thinking isn't bad.
Is a one liner, and is OK to render by default and hide easily setting fill to transparent.
You should go the extra mile and comment it clearly in the source code, that when adding a path to the text, that will be also drawn unless fill is transparent.

@melchiar
Copy link
Member Author

Sure, I can add some explanatory comments. My use case for this is as a resizing guide, drawing the path only during transform events.

text-path-resizing

@asturur
Copy link
Member

asturur commented Aug 26, 2021

We have e method called .isNotVisible that takes in account opacity and isVisible and some other little stuff.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 27, 2021

Code Coverage Summary

> fabric@4.5.1 coverage:report /home/runner/work/fabric.js/fabric.js
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.32 |    77.08 |   86.02 |   83.03 |                                               
 fabric.js |   83.32 |    77.08 |   86.02 |   83.03 | ...,29856,29981,30061-30126,30249,30348,30565 
-----------|---------|----------|---------|---------|-----------------------------------------------

asturur
asturur previously approved these changes Aug 27, 2021
@asturur
Copy link
Member

asturur commented Aug 27, 2021

i did not make a good choice with tests.
Do not merge before fixing tests

@github-actions
Copy link
Contributor

github-actions bot commented Aug 27, 2021

Code Coverage Summary

> fabric@4.5.1 coverage:report /home/runner/work/fabric.js/fabric.js
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.32 |    77.06 |   86.02 |   83.03 |                                               
 fabric.js |   83.32 |    77.06 |   86.02 |   83.03 | ...,29856,29981,30061-30126,30249,30348,30565 
-----------|---------|----------|---------|---------|-----------------------------------------------

@asturur
Copy link
Member

asturur commented Aug 27, 2021

Ok path opacity does not work, just a note does not have to be a bug.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 27, 2021

Code Coverage Summary

> fabric@4.5.1 coverage:report /home/runner/work/fabric.js/fabric.js
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.32 |    77.08 |   86.02 |   83.03 |                                               
 fabric.js |   83.32 |    77.08 |   86.02 |   83.03 | ...,29856,29981,30061-30126,30249,30348,30565 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Aug 27, 2021

Code Coverage Summary

> fabric@4.5.1 coverage:report /home/runner/work/fabric.js/fabric.js
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.32 |    77.06 |   86.02 |   83.03 |                                               
 fabric.js |   83.32 |    77.06 |   86.02 |   83.03 | ...,29856,29981,30061-30126,30249,30348,30565 
-----------|---------|----------|---------|---------|-----------------------------------------------

asturur
asturur previously approved these changes Aug 27, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Aug 27, 2021

Code Coverage Summary

> fabric@4.5.1 coverage:report /home/runner/work/fabric.js/fabric.js
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.31 |    77.07 |   86.02 |   83.03 |                                               
 fabric.js |   83.31 |    77.07 |   86.02 |   83.03 | ...,29875,30000,30080-30145,30268,30367,30584 
-----------|---------|----------|---------|---------|-----------------------------------------------

@asturur asturur merged commit 7243d83 into master Aug 27, 2021
@asturur asturur mentioned this pull request Aug 27, 2021
@eltaiguer
Copy link

Hi @asturur if I enliven a IText with a path, I get the following error
core.js:6014 ERROR TypeError: path.isNotVisible is not a function

Maybe there's a check missing or when re-building the IText the Path isn't being recreated?

@asturur
Copy link
Member

asturur commented Sep 26, 2021

Can you reproduce it in a fiddle?
Or can you post how are you enliving it?

@digitalml
Copy link

https://user-images.githubusercontent.com/9057412/130850444-8856caad-29ae-43f5-af3f-857b6fd82973.gif

@melchiar Would you be so kind as to share the fiddle for the above example you posted? I've checked here http://jsfiddle.net/user/melchiar/fiddles/ but don't see it. Thank you!

@melchiar
Copy link
Member Author

@digitalml Sure, I actually shared it in the main text on path issue tracker as well #6543 (comment)

https://jsfiddle.net/melchiar/vhgtesoy/

@digitalml
Copy link

@melchiar thank you. I did see this one. I was specifically looking for the hidden path version.

@melchiar
Copy link
Member Author

I haven't made a demo of that code since it's part of our platform, but it's just a matter of using events to show/hide the path during and after performing transformations.

@shyko
Copy link

shyko commented Mar 1, 2022

Hi @eltaiguer . I have the same problem - ERROR TypeError: path.isNotVisible is not a function when I use IText type. Did yuo find any solutions?

@melchiar
Copy link
Member Author

melchiar commented Mar 2, 2022

@shyko the path property is in beta and isn't currently supported for iText objects (just Text objects for now)

@Clemweb
Copy link

Clemweb commented Aug 17, 2023

@melchiar Hi, is it possible to have a Fiddle of the demo from the 25/08 ? The interaction is exactly what I need...

Thanks

@kalmigs
Copy link

kalmigs commented Dec 21, 2023

@digitalml Sure, I actually shared it in the main text on path issue tracker as well #6543 (comment)

https://jsfiddle.net/melchiar/vhgtesoy/

Thank for this @melchiar! this is what worked for me.
Been searching for this for days for curved text. Tried the old solutions but will not work on Node.js Canvas properly, Found out recently about Text paths and found this. Thank you so much!

jsfiddle is not working, but I just copied code in my project and works perfectly.

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.

7 participants