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

Improved AssetImage Docs #78173

Merged
merged 9 commits into from May 17, 2021
Merged

Conversation

Swayam221
Copy link
Contributor

@Swayam221 Swayam221 commented Mar 14, 2021

This PR aims at improving the api docs for AssetImage.
Fixes #78088

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Mar 14, 2021
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@@ -116,7 +116,11 @@ const double _kLowDprLimit = 2.0;
/// ```dart
/// AssetImage('icons/heart.png')
/// ```
/// See also:
Copy link
Contributor

Choose a reason for hiding this comment

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

blank line before paragraph

///
/// * [loading images](https://flutter.dev/docs/development/ui/assets-and-images#loading-images-1)
Copy link
Contributor

Choose a reason for hiding this comment

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

start each bullet with a captital letter

///
/// * [loading images](https://flutter.dev/docs/development/ui/assets-and-images#loading-images-1)
Copy link
Contributor

Choose a reason for hiding this comment

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

That link doesn't actually show how to use an AssetImage to get the asset, as far as i can tell. It shows how to use an AssetImage with an Image widget but that's already explained in the "See also" link at the bottom of this section.

What #78088 is asking for is for sample code showing how to actually get a ui.Image object out of an AssetImage.

@Piinks Piinks added d: api docs Issues with https://api.flutter.dev/ documentation labels Mar 17, 2021
@goderbauer
Copy link
Member

@Swayam221 Do you still have plans to come back to this PR?

@Swayam221
Copy link
Contributor Author

@Swayam221 Do you still have plans to come back to this PR?

@goderbauer Yes sir! I will submit a PR after making some amends by tonight .

@Swayam221
Copy link
Contributor Author

@Hixie Sir, pointed it to the image provider class, will this do?

@Swayam221 Swayam221 requested a review from Hixie March 25, 2021 20:28
@@ -116,7 +116,9 @@ const double _kLowDprLimit = 2.0;
/// ```dart
/// AssetImage('icons/heart.png')
/// ```
///
///
/// To understand how an [Image] object is made out of an [AssetImage], refer [ImageProvider] and [AssetBundleImageProvider].
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll have to adjust the Engilsh here for grammar and to fit the Flutter style, something more like:

/// For an example of resolving an [AssetImage] to obtain a [dart:ui.Image] object,
/// refer to the sample code in the documentation for [ImageProvider].

@Swayam221 Swayam221 requested a review from Hixie March 27, 2021 05:23
@goderbauer
Copy link
Member

@Swayam221 The analyzer check is reporting some unnecessary whitespace. Can you fix that up please?

@Swayam221
Copy link
Contributor Author

@Swayam221 The analyzer check is reporting some unnecessary whitespace. Can you fix that up please?

@goderbauer Fixed It Sir!

@Hixie
Copy link
Contributor

Hixie commented Apr 3, 2021

I recommend retriggering the tests, looks like you may have last updated the PR when the tree was broken in some way.

@Swayam221
Copy link
Contributor Author

I recommend retriggering the tests, looks like you may have last updated the PR when the tree was broken in some way.

@Hixie looks like tree is still broken?

@Hixie
Copy link
Contributor

Hixie commented Apr 5, 2021

Yeah, but flutter-build will fix itself. Once @goderbauer is happy, he'll put a label on the PR so it will land automatically when the build is green.

@goderbauer
Copy link
Member

@Swayam221 Do you still have plans to come back to this PR?

@Swayam221
Copy link
Contributor Author

@Swayam221 Do you still have plans to come back to this PR?

@goderbauer Yes sir. Sorry for the delays, I was involved with college work and was trying to apply for GSoC. I will try to submit a PR asap 😅.

@Swayam221
Copy link
Contributor Author

@goderbauer if the load() method in ImageProvider is responsible for fetching an image, will it be okay if I make it point to https://api.flutter.dev/flutter/painting/ImageProvider/load.html?

@Hixie
Copy link
Contributor

Hixie commented Apr 29, 2021

In the docs for ImageProvider there's a section labeled "Using an ImageProvider", which has sample code that explains how to use an ImageProvider to get a dart:ui.Image. Either some code similar to that, or, failing that, just pointing to that section of the docs, is what I had in mind when filing #78088.

@goderbauer
Copy link
Member

@Hixie Does the current version of this PR than answer your question?

@Hixie
Copy link
Contributor

Hixie commented Apr 29, 2021

Since the proposed sentence seemed unclear to you, it probably needs extra clarification. The easiest solution is probably just to put the same code here, updated to work with AssetImage specifically.

@Swayam221 Swayam221 requested a review from goderbauer May 5, 2021 11:04
@goderbauer
Copy link
Member

This is looking pretty good now. Can you rebase it to the latest master so all the checks will pass? Thanks.

@Swayam221
Copy link
Contributor Author

@goderbauer @Hixie Review Requested. Thanks.

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
d: api docs Issues with https://api.flutter.dev/ framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AssetImage docs improvements
5 participants