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

fix: Added useArtboardSize functionality #2294

Merged
merged 7 commits into from
Jan 28, 2023

Conversation

Anas35
Copy link
Contributor

@Anas35 Anas35 commented Jan 26, 2023

Description

Added useArtboardSize functionality. This is quite different than rive_flutter's useArtboardSize.

In rive_flutter, when useArtboardSize is true then the artboard size is constrained by the parent constraints.
since RiveComponent use PositionComponent which does not depends on parent constraints, artboard size here can take as much size as it required.

I don't know If this is the right approach, Feel free to correct me If I'm wrong.

Checklist

  • I have followed the [Contributor Guide] when preparing my PR.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples or docs.

Breaking Change?

  • Yes, this PR is a breaking change.
  • No, this PR is not a breaking change.

Related Issues

Closes #2058

@Anas35 Anas35 changed the title fix: added useArtboardSize functionality fix: Added useArtboardSize functionality Jan 26, 2023
Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Lgtm, simple and clean :)

@spydon spydon requested a review from a team January 26, 2023 15:20
Copy link
Member

@renancaraujo renancaraujo left a comment

Choose a reason for hiding this comment

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

Hello thanks for the PR.

This can lead to an inconsistent API.

The field useArtboardSize is used only in the constructor. As people can still change the size of the component during its lifetime, we could have a scenario where useArtboardSize is true but the sizes are different

@spydon
Copy link
Member

spydon commented Jan 27, 2023

Hello thanks for the PR.

This can lead to an inconsistent API.

The field useArtboardSize is used only in the constructor. As people can still change the size of the component during its lifetime, we could have a scenario where useArtboardSize is true but the sizes are different

It seems hard to account for that, maybe it should just be called initializeWithArtboardSize or something that indicates that it only takes the artboard size on creation?

@Anas35
Copy link
Contributor Author

Anas35 commented Jan 27, 2023

Yep, initializeWithArtboardSize could be one way to specify it, but It kinda gets redundant.

What about completely removing useArtboardSize and having the default value for size as artboard size? This would be breaking change only for those that didn't specify the size parameter.

@spydon
Copy link
Member

spydon commented Jan 27, 2023

Yep, initializeWithArtboardSize could be one way to specify it, but It kinda gets redundant.

What about completely removing useArtboardSize and having the default value for size as artboard size? This would be breaking change only for those that didn't specify the size parameter.

Sounds like a good idea, since it'll be very few that haven't specified it anyways since the default is (0, 0).

@spydon spydon merged commit 00b0dbe into flame-engine:main Jan 28, 2023
@spydon
Copy link
Member

spydon commented Jan 28, 2023

Thanks for your contribution @Anas35! 💙

@Anas35 Anas35 deleted the use_artboard_size branch January 28, 2023 15:08
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.

[flame_rive] functionality of useArtboardSize is missing
3 participants