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

Bug/4391 make markdown auto wrapping optional #4856

Conversation

dreathed
Copy link
Contributor

@dreathed dreathed commented Sep 19, 2023

📑 Summary

Hi! I'm new here and would like to be part of this awesome project! I'm still getting familiar with the code, so if I overlooked something, sorry in advance!
This PR solves the issue with auto wrapping markdown strings, by making auto wrapping optional for markdown strings.

Resolves #4391

📏 Design Decisions

I created a boolean option "markdownAutoWrap". If this option is set to false, then markdown strings are preprocessed, so that every whitespace is replaced by a " ". This way only explicit line breaks are rendered.

I added a unit test for the markdownToHTML function. I needed to import the setConfig function for testing.
I did not add a e2e test. BTW: 11 of 35 e2e tests fail. Can you tell me, whether this is a known issue or a problem on my side?

I updated the documentation for the flowchart.

I'm happy to get your feedback!

📋 Tasks

Make sure you

@netlify
Copy link

netlify bot commented Sep 19, 2023

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 16db0c0
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/65feb80daf24fc000887847f
😎 Deploy Preview https://deploy-preview-4856--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.

@dreathed dreathed marked this pull request as draft September 19, 2023 17:14
@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Attention: Patch coverage is 1.66667% with 59 lines in your changes are missing coverage. Please review.

Project coverage is 5.74%. Comparing base (ea86697) to head (16db0c0).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #4856      +/-   ##
==========================================
- Coverage     5.75%   5.74%   -0.01%     
==========================================
  Files          277     277              
  Lines        41726   41743      +17     
  Branches        27      27              
==========================================
  Hits          2400    2400              
- Misses       39325   39342      +17     
  Partials         1       1              
Flag Coverage Δ
unit 5.74% <1.66%> (-0.01%) ⬇️

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

Files Coverage Δ
packages/mermaid/src/config.ts 52.22% <100.00%> (ø)
packages/mermaid/src/dagre-wrapper/clusters.js 0.00% <0.00%> (ø)
packages/mermaid/src/diagrams/common/common.ts 52.50% <0.00%> (+0.27%) ⬆️
packages/mermaid/src/rendering-util/createText.ts 0.00% <0.00%> (ø)
packages/mermaid/src/diagrams/mindmap/svgDraw.ts 0.00% <0.00%> (ø)
...mermaid/src/rendering-util/handle-markdown-text.ts 0.00% <0.00%> (ø)
packages/mermaid/src/dagre-wrapper/edges.js 0.00% <0.00%> (ø)
packages/mermaid/src/dagre-wrapper/shapes/util.js 0.00% <0.00%> (ø)

@dreathed dreathed marked this pull request as ready for review September 19, 2023 18:48
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.

Nice work @dreathed !

We'll have to remove the setConfig and getConfig imports, by passing the parameter inside an options object.

    markdownToHTML(`Hello, how do you do?`, { markdownAutoWrap: false });

The e2e tests should not be failing.
Can you add an imgSnapshotTest anyway, we'll verify that here.

packages/mermaid/src/docs/syntax/flowchart.md Outdated Show resolved Hide resolved
* develop: (935 commits)
  chore: Minor fixes
  chore: Build docs
  Use develop as base on develop branch.
  Update renovate json
  update link
  update announcement and blog pages
  Remove `--force` flag
  Tweak editor.bash
  update link
  chore: update browsers list
  Update integrations-community: add Drupal and module.
  Support Firefox
  Address review comments
  Change run symbol
  feat: Make the examples interactive in the documentation site.
  Add langium
  chore: update browsers list
  chore(deps): update all patch dependencies
  chore(deps): update all minor dependencies
  Update keywords and description
  ...
@github-actions github-actions bot added the Type: Bug / Error Something isn't working or is incorrect label Mar 23, 2024
* develop: (21 commits)
  Linting
  chore:  temp fix for eslint OOM
  chore: Update error snapshots
  Fix ESLint
  chore: Prettier
  chore: YOLO `pnpm --recursive update`
  chore: Remove commitlint
  Fix flowchart-elk render test
  chore: Add example page link in index
  chore: Remove cy.viewport
  fix: Cypress test for Suppress Error
  fix: Race condition when running suppressError test.
  fix: Retain default behavior when rendering errors cases
  chore: Add suppressErrorRendering to secure flags.
  fix: Remove blank SVG
  docs: Rebuild
  Add test
  Add test
  chore: Add suppressErrorRendering to config
  Throw error when detecting diagram type failed and `suppressErrorRendering` is set
  ...
* develop:
  Wait for image to be rendered
  Fix flowchart-elk render test
  chore: Add example page link in index
  fix: ELK diagram remove re-parsing
  Added linting
  fixed title bounds calculation, removed extra title from merging issues
  Centered Title function and changed rendering order for Elk flowchart to find Boundingbox
  Fixed styling for lines for ELK flowchart
@sidharthv96 sidharthv96 merged commit 223f339 into mermaid-js:develop Mar 23, 2024
18 of 19 checks passed
sidharthv96 added a commit that referenced this pull request Mar 23, 2024
…in-class-name

* develop: (501 commits)
  Add extra test
  Add visual test
  Wait for image to be rendered
  Update docs
  Linting
  chore:  temp fix for eslint OOM
  chore: Update error snapshots
  Fix ESLint
  chore: Prettier
  chore: YOLO `pnpm --recursive update`
  chore: Remove commitlint
  Fix flowchart-elk render test
  chore: Add example page link in index
  Fix flowchart-elk render test
  chore: Add example page link in index
  fix: Remove space which caused extra newline on diagrams
  fix: Remove repeated config calls
  fix: ELK diagram remove re-parsing
  chore: Minor fixes #4856
  chore: Increase ESLint memory limit
  ...
sidharthv96 added a commit that referenced this pull request Mar 23, 2024
* develop: (101 commits)
  Add extra test
  Add visual test
  Wait for image to be rendered
  Update docs
  Linting
  chore:  temp fix for eslint OOM
  chore: Update error snapshots
  Fix ESLint
  chore: Prettier
  chore: YOLO `pnpm --recursive update`
  chore: Remove commitlint
  Fix flowchart-elk render test
  chore: Add example page link in index
  Fix flowchart-elk render test
  chore: Add example page link in index
  fix: Remove space which caused extra newline on diagrams
  fix: Remove repeated config calls
  fix: ELK diagram remove re-parsing
  chore: Minor fixes #4856
  chore: Increase ESLint memory limit
  ...
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.

Markdown strings formatting without automatic text wrapping
3 participants