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

[flutter_markdown] Added sizedImageBuilder to Markdown widget #6739

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

Conversation

M97Chahboun
Copy link

@M97Chahboun M97Chahboun commented May 15, 2024

  • Adds sizedImageBuilder to Markdown widget.

Pre-launch Checklist

Copy link
Contributor

@domesticmouse domesticmouse left a comment

Choose a reason for hiding this comment

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

I'm curious how image height and width are parsed from the markdown.

@M97Chahboun
Copy link
Author

M97Chahboun commented May 18, 2024

@domesticmouse
If you means how markdown parse the h & w of image
check this line
parsed by get height and width from this format imageLink#200x200

@M97Chahboun M97Chahboun requested a review from domesticmouse May 18, 2024 20:19
@domesticmouse
Copy link
Contributor

Please add a test that covers width x height from markdown to final usage to prevent future breakage

@stuartmorgan
Copy link
Contributor

  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]

This was not done; please don't check boxes without completing the steps they describe.

  • All existing and new tests are passing.

CI indicates that the new test is not passing, which will need to be resolved before this can be reviewed.

@M97Chahboun M97Chahboun changed the title Added height and width to image builder [flutter_markdown] Added height and width to image builder Jun 4, 2024
@M97Chahboun
Copy link
Author

M97Chahboun commented Jun 5, 2024

I run image tests before any changes and I get :

══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following assertion was thrown while running async test code:
Golden "assets/images/golden/image_test/resource_asset_logo.png": Pixel test failed, 0.00% diff
detected.
Failure feedback can be found at
/Users/user/Development/packages/packages/flutter_markdown/test/failures

When the exception was thrown, this was the stack:
#0      LocalFileComparator.compare (package:flutter_test/src/_goldens_io.dart:106:5)
<asynchronous suspension>
#1      MatchesGoldenFile.matchAsync.<anonymous closure> (package:flutter_test/src/_matchers_io.dart:118:32)
<asynchronous suspension>
<asynchronous suspension>
(elided one frame from dart:async)

The exception was caught asynchronously.
════════════════════════════════════════════

Any suggestion please

@stuartmorgan
Copy link
Contributor

Tiny changes in golden files are normal if you are on a different host OS than the baseline (which is usually Linux). The issue on CI though is that you've dramatically changed an existing golden test, but haven't changed the expected output.

Please make a new test instead of changing that one, and set a new golden for it.

@stuartmorgan stuartmorgan requested a review from tarrinneal July 23, 2024 19:51
@stuartmorgan
Copy link
Contributor

From triage: @M97Chahboun Are you planning on updating the tests per the comments above?

@M97Chahboun
Copy link
Author

M97Chahboun commented Sep 3, 2024

@stuartmorgan I don't have any idea about golden test, but I will try to implement it ASAP

@stuartmorgan stuartmorgan marked this pull request as draft October 1, 2024 19:55
@stuartmorgan
Copy link
Contributor

Marking as a draft pending new, passing tests that can be reviewed.

@stuartmorgan
Copy link
Contributor

Since this is marked as a draft and hasn't been updated in several months, I’m going to close it so that our PR queue reflects active PRs. Please don't hesitate to submit a new PR if you decide to revisit this. Thanks!

@stuartmorgan
Copy link
Contributor

From triage: @domesticmouse it looks like this is ready for re-review.

Copy link
Contributor

@domesticmouse domesticmouse left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@tarrinneal tarrinneal left a comment

Choose a reason for hiding this comment

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

You'll need to run the format command as well. Probably good to run the analyzer as well to check for issues.

I haven't looked into your other failing tests. You can click on them and look at what's failing, or run the tests locally to debug them.

@stuartmorgan
Copy link
Contributor

@M97Chahboun The test failures are in your new test; merging in main isn't going to resolve them, it's just unnecessarily triggering all of the CI again each time.

Copy link
Contributor

@tarrinneal tarrinneal left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for putting this together!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants