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

Backdrop filters #196

Merged
merged 42 commits into from
Mar 2, 2022
Merged

Backdrop filters #196

merged 42 commits into from
Mar 2, 2022

Conversation

wcandillon
Copy link
Contributor

@wcandillon wcandillon commented Feb 12, 2022

Add the possibility to create backdrop filters.
We're taking advantage of this new feature to clean up shape processing.

  • Clean up shape processing (rect, rrect, path, clips)
  • Document
  • Align API with Flutter
  • Remove color property in backdropBlur (clashing with color property in Group)
  • Add default parameters for the offset filter (x=0, y=0)

@wcandillon wcandillon self-assigned this Feb 12, 2022
@wcandillon wcandillon requested a review from chrfalch February 12, 2022 13:52
@wcandillon
Copy link
Contributor Author

@chrfalch Ready to review. This PR also contains a small clean up of some of the "processors" (Colors, Paths, RRect, Clips).

@wcandillon wcandillon marked this pull request as ready for review February 12, 2022 13:54
docs/docs/backdrops.md Outdated Show resolved Hide resolved
docs/docs/group.md Outdated Show resolved Hide resolved
@chrfalch

This comment was marked as resolved.

Copy link
Contributor

@chrfalch chrfalch left a comment

Choose a reason for hiding this comment

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

I have a few comments about the I prefix on types (in relation to our discussion about the same in the animation PR). Should we avoid this all over?

Also some typos in the documentation, and I'd also like to add a description of what a backdrop is in the docs.

example/ios/Podfile.lock Outdated Show resolved Hide resolved
example/ios/RNSkia.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
package/cpp/api/JsiSkCanvas.h Show resolved Hide resolved
package/src/skia/Canvas.ts Show resolved Hide resolved
package/src/skia/Skia.ts Show resolved Hide resolved
Copy link
Contributor

@chrfalch chrfalch left a comment

Choose a reason for hiding this comment

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

  • Remove changes in Podfile.lock
  • Remove changes in project.pbxproj

@wcandillon wcandillon requested a review from chrfalch February 28, 2022 10:03
@wcandillon
Copy link
Contributor Author

@chrfalch Ready for review 🙋🏼‍♂️ I'm still confused about the current state of the podfile lock but we will discuss it offline.

docs/docs/backdrop-filters.md Show resolved Hide resolved
({ canvas, paint }, { start, end, stroke, path: pathDef }) => {
const path = processPath(pathDef);
({ canvas, paint }, { start, end, stroke, ...pathProps }) => {
const path = processPath(pathProps.path).copy();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we do copy here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

trim() is mutating the path.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if I understand - if trim is used inside processPath, maybe processPath should be the one doing the copy operation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the trim is not used in processPath rather in this component (in the next step)

export interface ImageFilterFactory {
MakeOffset(dx: number, dy: number, input: IImageFilter | null): IImageFilter;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be correct to put documentation / comments on these methods here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes 100%

@wcandillon wcandillon merged commit 179fcb2 into main Mar 2, 2022
@wcandillon wcandillon deleted the feat/glassmorphism branch March 14, 2022 08:51
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.

2 participants