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

fix: flowchart image without text #5063

Merged

Conversation

bonyuta0204
Copy link
Contributor

@bonyuta0204 bonyuta0204 commented Nov 23, 2023

📑 Summary

Resolved the issue with image size when utilizing only the <img> tag in nodes within a flowchart diagram.

Resolves #4736

📏 Design Decisions

I have implemented a solution where the size of the <img> tag is defined using both max-width and min-width properties, specifically in scenarios where a node contains only the <img> tag. This approach addresses the issue where merely setting the width was insufficient.

📋 Tasks

Make sure you

Copy link

netlify bot commented Nov 23, 2023

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 1ff7218
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/656098b2fd370d0009d08aed
😎 Deploy Preview https://deploy-preview-5063--mermaid-js.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added the Type: Bug / Error Something isn't working or is incorrect label Nov 23, 2023
Copy link

codecov bot commented Nov 23, 2023

Codecov Report

Merging #5063 (1ff7218) into develop (61747b6) will increase coverage by 36.38%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           develop    #5063       +/-   ##
============================================
+ Coverage    42.91%   79.29%   +36.38%     
============================================
  Files           23      164      +141     
  Lines         5029    13884     +8855     
  Branches        21      700      +679     
============================================
+ Hits          2158    11009     +8851     
+ Misses        2870     2725      -145     
- Partials         1      150      +149     
Flag Coverage Δ
e2e 85.17% <100.00%> (?)
unit 42.91% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
packages/mermaid/src/dagre-wrapper/shapes/util.js 96.36% <100.00%> (ø)

... and 158 files with indirect coverage changes

@bonyuta0204 bonyuta0204 force-pushed the bug/4736_zero_size_image_flowchart branch from 9cf21b5 to 9b4e4ae Compare November 23, 2023 15:10
@bonyuta0204 bonyuta0204 marked this pull request as ready for review November 24, 2023 12:32
@bonyuta0204 bonyuta0204 force-pushed the bug/4736_zero_size_image_flowchart branch from 9b4e4ae to 1ff7218 Compare November 24, 2023 12:36
@sidharthv96
Copy link
Member

@bonyuta0204 it doesn't seem to be working for me. Link

Can you post an example that's working in https://deploy-preview-5063--mermaid-js.netlify.app ?

@bonyuta0204
Copy link
Contributor Author

@sidharthv96
Thank you for reviewing this pull request.

In my environment, everything seems to be functioning correctly.

Screenshot of the application running in my environment
Note: I've used the link that you provided.

Here are the details of my environment:

  • MacOS 13.6
  • Google Chrome 119.0.6045.159 (Official Build) (arm64)

Could you please share the details of your environment? I would like to investigate further.

@sidharthv96
Copy link
Member

Ok, so this might be a Firefox bug then?

I'm using FF 118.0.1 on MacOS 13.3.1 (a)

Copy link
Member

@sidharthv96 sidharthv96 left a comment

Choose a reason for hiding this comment

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

Issue is still present in Firefox, but working in Chrome and Safari. Nice work @bonyuta0204.

Is the maxWidth required?

@bonyuta0204
Copy link
Contributor Author

bonyuta0204 commented Nov 27, 2023

@sidharthv96
Thank you. I will investigate the issue with Firefox when I have time. (I will submit another pull request once I have a solution.)

Is the maxWidth required?

Yes, maxWidth is required. Without it, images could expand to their intrinsic size, which might be much larger than the specified size.

@sidharthv96 sidharthv96 added this pull request to the merge queue Nov 27, 2023
Merged via the queue into mermaid-js:develop with commit 7ca76b0 Nov 27, 2023
18 checks passed
Copy link

mermaid-bot bot commented Nov 27, 2023

@bonyuta0204, Thank you for the contribution!
You are now eligible for a year of Premium account on MermaidChart.
Sign up with your GitHub account to activate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug / Error Something isn't working or is incorrect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image stop rendering after 9.1 (2º Issue)
2 participants