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

Allow Colon Characters in Period, Sections, and Event Text #5418

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

FutzMonitor
Copy link
Contributor

@FutzMonitor FutzMonitor commented Mar 26, 2024

📑 Summary

This PR seeks to allow users to include colon characters in their period, section, and event element text.

Resolves #4175

📏 Design Decisions

  1. I added a new function replaceEscapedColon() which transforms ESC_COLON into a colon character.
  2. I made changes to the documentation to guide users on how to include colon characters in their period, section, and event element text while also letting users know that this cannot be done for titles.

📋 Tasks

Make sure you

1. Added function replaceEscapedColon() to replace all instances of 'ESC_COLON' with colons.
2. Added a call to this function within the drawText() function to replace any text that contains  to a colon.
3. AUTHOR'S NOTE: These changes only apply for sections, periods, and event elements. This functionality does not work for timeline titles.
Changes to timeline.md
1. Updated documentation to guide users on how to include colon characters in their text.
Copy link

netlify bot commented Mar 26, 2024

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit e529cbd
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/660c9b169fae150009a5497a
😎 Deploy Preview https://deploy-preview-5418--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.

@FutzMonitor
Copy link
Contributor Author

I have not added the necessary tests for this change to the timeline diagram. I was hoping to get some pointers on how to create tests for this new functionality because the timeline diagram has no rendering tests like some other diagrams. The timeline.spec.js file currently only tests the parser but this functionality is implemented during rendering.

Copy link

codecov bot commented Mar 26, 2024

Codecov Report

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

Project coverage is 5.74%. Comparing base (73c426f) to head (e529cbd).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #5418      +/-   ##
==========================================
- Coverage     5.74%   5.74%   -0.01%     
==========================================
  Files          276     277       +1     
  Lines        41887   41908      +21     
  Branches       514     489      -25     
==========================================
  Hits          2407    2407              
- Misses       39480   39501      +21     
Flag Coverage Δ
unit 5.74% <0.00%> (-0.01%) ⬇️

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

Files Coverage Δ
packages/mermaid/src/diagrams/timeline/svgDraw.js 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

@sidharthv96
Copy link
Member

@FutzMonitor I see some rendering tests in cypress/integration/rendering/timeline.spec.ts.

@FutzMonitor
Copy link
Contributor Author

@FutzMonitor I see some rendering tests in cypress/integration/rendering/timeline.spec.ts.

Thank you, I'll take a look at it and add a test for my changes.

1. Added test coverage to rendering changes where 'ESC_COLON' should be changed to a colon character and this change should not break rendering behavior.
@FutzMonitor FutzMonitor marked this pull request as ready for review March 30, 2024 19:06
1. Changes the test title because it was previously inaccurate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to escape : in timeline
2 participants