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

Added borderColor #58

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

ghost
Copy link

@ghost ghost commented Jan 5, 2022

When borderColor is set, a border with the given color is added to the image exterior, making it better to see the image ending when the baseColor and the image background are similar

demo:
borderColor_demonstration

When borderColor is set, a border with the given color is added to the image exterior, making it better to see the image ending when the baseColor and the image background are similar
@gabriel-cervantes
Copy link

Very good!!

@luciano-cervantes
Copy link

what a good guy. @vinicios-cervantes

@luciano-cervantes
Copy link

Hello @chooyan-eng, please review this PR

@chooyan-eng
Copy link
Owner

@luciano-cervantes
Hi, thank you very much for your PR!

As I have few time to maintain this package, I've now started to check the PR.

As long as reading your PR description and capture, it's nice idea! Wait for my checking your code.

@ghost
Copy link
Author

ghost commented May 18, 2022

Still waiting...

@luciano-cervantes
Copy link

Complicated...

@ghost
Copy link
Author

ghost commented Oct 17, 2022

Very complicated @luciano-cervantes

@luciano-cervantes
Copy link

The best review ever done... @vinicios-cervantes
Alt Text

@luciano-cervantes
Copy link

It has already completed a birthday @vinicios-cervantes . 🎉🎉

@gabriel-cervantes
Copy link

gabriel-cervantes commented Jan 11, 2023

Happy birthday!! 🎂🥳🎉

Alt Text

Copy link
Owner

@chooyan-eng chooyan-eng left a comment

Choose a reason for hiding this comment

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

@vinicios-cervantes
I really apologize for leaving this PR for more than a year!
As I checked and commented some points, check and fix (or leave your comments to) them. Thanks.

this.fixArea = false,
this.progressIndicator = const SizedBox.shrink(),
this.interactive = false,
}) : assert((initialSize ?? 1.0) <= 1.0,
'initialSize must be less than 1.0, or null meaning not specified.'),
}) : assert((initialSize ?? 1.0) <= 1.0, 'initialSize must be less than 1.0, or null meaning not specified.'),
Copy link
Owner

Choose a reason for hiding this comment

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

Can you format the code so that unrelated changes are not included in this PR?

? MediaQuery.of(context).size.height * _scale
: null,
fit: BoxFit.contain,
child: Flex(
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need Flex? It seems children always has only one child and Flex is not necessary.

Positioned(
left: _imageRect.left,
top: _imageRect.top,
child: Image.memory(
Copy link
Owner

Choose a reason for hiding this comment

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

I guess the border decoration should come here. Otherwise, as long as checking the example app, the border comes to the boundary of Crop, not the image.

/// When [borderColor] is set, a border with the given color is added to the
/// image exterior, making it better to see the image ending when
/// the [baseColor] and the image background are similar
final Color? borderColor;
Copy link
Owner

Choose a reason for hiding this comment

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

Why don't we accept Border, not only Color?
It enables users to decide other configurations, such as width.

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

4 participants