Skip to content

Image attributes - #497

Closed
marcio-ota wants to merge 5 commits into
fleather-editor:masterfrom
marcio-ota:image-attributes
Closed

Image attributes#497
marcio-ota wants to merge 5 commits into
fleather-editor:masterfrom
marcio-ota:image-attributes

Conversation

@marcio-ota

@marcio-ota marcio-ota commented May 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Added image atributes.Image attributes for HTML tag compatibility:

<img src="http://fake.link/image.png" width="100" height="100">

BlockEmbed.image({
  'http://fake.link/image.png',
  'width': 100,
  'height': 100
});

@codecov

codecov Bot commented May 5, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.42857% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.93%. Comparing base (951034e) to head (363a84a).
⚠️ Report is 17 commits behind head on master.

Files with missing lines Patch % Lines
packages/parchment/lib/src/codecs/html.dart 83.33% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #497      +/-   ##
==========================================
+ Coverage   88.50%   88.93%   +0.43%     
==========================================
  Files          62       63       +1     
  Lines       10803    10903     +100     
==========================================
+ Hits         9561     9697     +136     
+ Misses       1242     1206      -36

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@amantoux
amantoux marked this pull request as ready for review May 9, 2025 07:30
@amantoux
amantoux marked this pull request as draft May 9, 2025 07:32
@amantoux

amantoux commented Jun 11, 2025

Copy link
Copy Markdown
Member

@marcio-ota is this PR still is draft state?

@marcio-ota

marcio-ota commented Jun 11, 2025

Copy link
Copy Markdown
Contributor Author

Hi @amantoux, Is everything ok with the PR? Any other suggestions?

@amantoux

amantoux commented Jun 11, 2025

Copy link
Copy Markdown
Member

Hi @amantoux, Is everything ok with the PR? Any other suggestions?

It was still tagged as draft so we did not review it
You confirm it is ready for review?

@marcio-ota
marcio-ota marked this pull request as ready for review June 11, 2025 12:45
@amantoux
amantoux self-requested a review July 5, 2025 06:37
amantoux
amantoux requested changes Jul 5, 2025
View reviewed changes

@amantoux amantoux left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for the PR @marcio-ota.
I left some comments, can you please take a look?

@Amir-P Any additional comment?

Comment thread packages/parchment/lib/src/document/embeds.dart Outdated
static final BlockEmbed horizontalRule = BlockEmbed('hr');
static BlockEmbed image(String source) =>
BlockEmbed('image', data: {'source': source});
static BlockEmbed image(String source, Map<String, dynamic> data) {

@amantoux amantoux Jul 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is introducing a breaking change in the API, can data be an optional parameter?

@amantoux amantoux Aug 1, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@marcio-ota It doesn't seem you have addresses this comment

Comment thread packages/parchment/lib/src/codecs/html.dart
@amantoux
amantoux requested a review from Amir-P July 5, 2025 06:41
Amir-P
Amir-P reviewed Jul 5, 2025
View reviewed changes
Comment thread packages/parchment/lib/src/document/embeds.dart Outdated
Comment on lines +101 to +105
static BlockEmbed image(String source, Map<String, dynamic> data) {
final newData = Map<String, dynamic>.from(data);
newData['source'] = source;
return BlockEmbed('image', data: newData);
}

@Amir-P Amir-P Jul 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry for the late review and thanks for contributing @marcio-ota! In addition to @amantoux comments, I have one tiny simplification suggestion:

Suggested change
static BlockEmbed image(String source, Map<String, dynamic> data) {
final newData = Map<String, dynamic>.from(data);
newData['source'] = source;
return BlockEmbed('image', data: newData);
}
static BlockEmbed image(String source, Map<String, dynamic> data) =>
BlockEmbed('image', data: {...data, 'source': source});

@amantoux

amantoux commented Jul 30, 2025

Copy link
Copy Markdown
Member

@marcio-ota Any chance to move forward on this PR? It would be great?
Thanls

@marcio-ota
marcio-ota requested a review from amantoux August 1, 2025 02:20
amantoux
amantoux requested changes Aug 1, 2025
View reviewed changes

@amantoux amantoux left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@marcio-ota Can you please address the comment? many thanks

Comment thread packages/parchment/lib/src/codecs/html.dart
Comment thread packages/parchment/lib/src/document/embeds.dart Outdated
static final BlockEmbed horizontalRule = BlockEmbed('hr');
static BlockEmbed image(String source) =>
BlockEmbed('image', data: {'source': source});
static BlockEmbed image(String source, Map<String, dynamic> data) {

@amantoux amantoux Aug 1, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@marcio-ota It doesn't seem you have addresses this comment

marcio-ota
marcio-ota commented Aug 1, 2025
View reviewed changes

@marcio-ota marcio-ota left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Add image attribute to Parchment codec

@amantoux

amantoux commented Aug 4, 2025

Copy link
Copy Markdown
Member

Add image attribute to Parchment codec

@marcio-ota you're last commit was empty, is it intended?

@marcio-ota

marcio-ota commented Aug 5, 2025

Copy link
Copy Markdown
Contributor Author

What do I need to do now? :-)

@amantoux

amantoux commented Aug 5, 2025

Copy link
Copy Markdown
Member

What do I need to do now? :-)

I can co-author if you want, let me know

@amantoux
amantoux self-requested a review August 8, 2025 10:06
@amantoux
amantoux requested a review from Amir-P August 8, 2025 10:07
@amantoux

amantoux commented Aug 8, 2025

Copy link
Copy Markdown
Member

@Amir-P can you please take a look?

@marcio-ota marcio-ota closed this by deleting the head repository Aug 11, 2025
@amantoux

amantoux commented Aug 11, 2025

Copy link
Copy Markdown
Member

@marcio-ota Why are you closing this PR? We are close to merging

@amantoux amantoux mentioned this pull request Aug 11, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@amantoux amantoux amantoux approved these changes
@Amir-P Amir-P Awaiting requested review from Amir-P

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@marcio-ota @amantoux @Amir-P