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(): dataURL export - filter objects #7788

Merged
merged 3 commits into from
Mar 9, 2022
Merged

Conversation

ShaMan123
Copy link
Contributor

Added a nice and simple option to filter objects for canvas export

This option is shallow, it doesn't filter objects under groups, yet...

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.18 |     76.6 |   86.38 |    82.9 |                                               
 fabric.js |   83.18 |     76.6 |   86.38 |    82.9 | ...,29868,29993,30073-30138,30261,30360,30577 
-----------|---------|----------|---------|---------|-----------------------------------------------

Copy link
Member

@melchiar melchiar left a comment

Choose a reason for hiding this comment

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

this is great!

@ShaMan123 ShaMan123 linked an issue Mar 9, 2022 that may be closed by this pull request
@ShaMan123
Copy link
Contributor Author

Thanks. So should I merge?

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Mar 9, 2022

I am thinking of supporting group as well.
But it might be simpler to clone the group and run toDataURL on the clone NOT supporting it under the hood.
What do you think?

@melchiar
Copy link
Member

melchiar commented Mar 9, 2022

Thanks. So should I merge?

looks good to me

I am thinking of supporting group as well. But it might be simpler to clone the group and run toDataURL on the clone NOT supporting it under the hood. What do you think?

sounds good, as I recall that's how toDataURL works on a fabric canvas as well

@ShaMan123 ShaMan123 merged commit b2d6dd9 into master Mar 9, 2022
@ShaMan123 ShaMan123 deleted the feat-filter-objects-export branch March 9, 2022 07:37
@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Mar 9, 2022

sounds good, as I recall that's how toDataURL works on a fabric canvas as well

It uses exiting objects on a new canvas
New group can easily accept a filtering function, basically it already does.

ShaMan123 added a commit that referenced this pull request Mar 10, 2022
* feat(): filter options

* visual test
ShaMan123 added a commit to amirhossein92/fabric.js that referenced this pull request Mar 11, 2022
commit ddda273
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Fri Mar 11 10:54:28 2022 +0200

    fix selection cases

commit fdba87f
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Fri Mar 11 10:54:14 2022 +0200

    rename

commit 0551fe0
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Fri Mar 11 10:28:49 2022 +0200

    forward/backward

commit befdcd8
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Fri Mar 11 10:00:48 2022 +0200

    rtl support prep

commit 36bbcb1
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Fri Mar 11 09:20:32 2022 +0200

    Update itext.class.js

commit c021097
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Fri Mar 11 09:03:07 2022 +0200

    feat(IText): selectionDirection, selectionDirection

commit 4f9529e
Author: amir hossein <a.mehrabi.70@gmail.com>
Date:   Wed Feb 16 09:46:27 2022 +0330

    fix(fabric.Text): consider justify text alignment in RTL text

commit fb3054b
Author: amir hossein <a.mehrabi.70@gmail.com>
Date:   Sat Feb 12 13:23:07 2022 +0330

    fix(fabric.Text): support RTL different text alignments

commit 07f597d
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Thu Mar 10 20:28:48 2022 +0200

    Update text.js

commit 28596f5
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Thu Mar 10 20:17:45 2022 +0200

    fix(tests)

commit 681c019
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Thu Mar 10 20:05:57 2022 +0200

    pathSide

commit 1a3f138
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Thu Mar 10 19:40:40 2022 +0200

    lint + rename

commit 3b80c7b
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Thu Mar 10 19:26:21 2022 +0200

    cleanup

commit 76f94b4
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Thu Mar 10 18:56:00 2022 +0200

    feat(Text): textAlign `start`, `end`

    and a bit of tidying up

commit 2e608dc
Author: Shachar <34343793+ShaMan123@users.noreply.github.com>
Date:   Wed Mar 9 09:37:43 2022 +0200

    feat(): dataURL export - filter objects (fabricjs#7788)

    * feat(): filter options

    * visual test

commit 1ea2465
Author: Amirhossein Mehrabi <a.mehrabi.70@gmail.com>
Date:   Sat Feb 26 22:36:46 2022 +0330

    fix(fabric.Text): add the previous code as comment
@salomonsanz
Copy link

salomonsanz commented Mar 11, 2022

"excludeFromExport" already exists. In the future, to "centralize" all export options, it might be nice to accept an object instead of a boolean

excludeFromExport = {
    json : true,
    dataUrl : true,
    svg : true
}

Even a filter function

excludeFromExport = {
    json : obj=>...,
    dataUrl : obj=>...,
    svg : obj=>...,
}

@ShaMan123
Copy link
Contributor Author

I didn't think of excludeFromExport. Good point.
I'd prefer removing this strange but useful prop in favor of filtering functions everywhere.

@salomonsanz
Copy link

mmm, from my experience, most filters of that type are based on object properties, not on dynamic logic, so in the end it would be redundant to delete that property and pass everything to functions:

objects.filter(o=>o.excludeFromExport)

On the other hand, there is no need to choose between functions and boolean properties, in many frameworks a design pattern that allows a value or a function is common.

https://laravel.com/docs/9.x/helpers#method-value

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Mar 13, 2022

I want to make fabric more intuitive. That's why excludeFromExport is on my target (e.g. #7719).
As you said, it is unclear how it should behave in some cases. Should it apply for every export? toCanvasElement , toDataURL, toJSON, clone, cloneAsImage. A filter function, however, it very clear and easy to maintain.

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.

Object.toDataURL include intersecting objects
3 participants