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

Implement HeroProperties #177

Merged
merged 2 commits into from Sep 3, 2019
Merged

Implement HeroProperties #177

merged 2 commits into from Sep 3, 2019

Conversation

ghost
Copy link

@ghost ghost commented Aug 29, 2019

Please refer to #175.

Hugo Passos added 2 commits August 29, 2019 12:17
Effective dart: PREFER relative paths when importing libraries within your own package’s lib directory.
You can check it here: https://dart.dev/guides/language/effective-dart/usage#prefer-relative-paths-when-importing-libraries-within-your-own-packages-lib-directory
Please refer to #175.
@@ -153,9 +154,6 @@ class PhotoViewGallery extends StatefulWidget {
/// Mirror to [PhotoView.enableRotation]
final bool enableRotation;

/// Mirror to [PhotoView.transitionOnUserGestures]
final bool transitionOnUserGestures;

Copy link
Author

@ghost ghost Aug 29, 2019

Choose a reason for hiding this comment

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

Seems like this is a negative side effect of this PR. Previously, the user could define transitionOnUserGestures to all PhotoViewGallery pages, but now they must define it to each page. However, I think most people use PhotoViewGallery.builder(), so they won't need to define transitionOnUserGestures more than once, anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, not that bad, but since it is a breaking change, this is gonna require a major version, thanks.

HeroAttributes get heroAttributes => widget.heroAttributes;

PhotoViewControllerDelegate get delegate => widget.delegate;

Copy link
Author

Choose a reason for hiding this comment

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

I also created those getters for Widget attributes that are used very often.

Copy link
Member

Choose a reason for hiding this comment

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

Great! I've detecetd an memory leak here involving delegate, but for now this is going in.

onTapUp: widget.onTapUp == null ? null : onTapUp,
onTapDown: widget.onTapDown == null ? null : onTapDown,
onTapUp: onTapUp,
onTapDown: onTapDown,
Copy link
Author

Choose a reason for hiding this comment

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

Just a tiny detail. Instead dealing with null callbacks here, I created null-aware function calls inside onTapUp() and onTapDown().

Copy link
Member

Choose a reason for hiding this comment

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

Topzera mano

@ghost
Copy link
Author

ghost commented Aug 30, 2019

The tests are passing and the examples are running, so I have no idea why CI is failing. If you could point me out on how to solve this, I'd work on it.

@renancaraujo
Copy link
Member

The tests are passing and the examples are running, so I have no idea why CI is failing. If you could point me out on how to solve this, I'd work on it.

Dont worry, im about to change how we are integrating this. LGTM.

@renancaraujo renancaraujo merged commit 5e45056 into bluefireteam:master Sep 3, 2019
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.

None yet

1 participant