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

Show Full Title in Pie Chart SVG #5596

Closed
wants to merge 7 commits into from
Closed

Show Full Title in Pie Chart SVG #5596

wants to merge 7 commits into from

Conversation

yari-dewalt
Copy link
Collaborator

📑 Summary

Aims to fix the title of a pie chart overflowing outside the bounds of the rendered SVG.

Resolves #5567

📏 Design Decisions

The goal was to keep the rendering the same but to account for the title length if it surpasses the length of the overall diagram. If so, it will update the total width of the diagram and offset the chart so it remains centered under the title text. It also maintains the margin on both sides.

📋 Tasks

Make sure you

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

netlify bot commented Jun 24, 2024

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 3a7fc6e
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/6697b1927ea28d0008979c63
😎 Deploy Preview https://deploy-preview-5596--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.

Copy link

codecov bot commented Jun 24, 2024

Codecov Report

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

Project coverage is 5.85%. Comparing base (b1b480a) to head (3a7fc6e).
Report is 394 commits behind head on develop.

Files Patch % Lines
packages/mermaid/src/diagrams/pie/pieRenderer.ts 0.00% 21 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #5596      +/-   ##
==========================================
- Coverage     5.86%   5.85%   -0.01%     
==========================================
  Files          274     274              
  Lines        41087   41105      +18     
  Branches       488     488              
==========================================
  Hits          2408    2408              
- Misses       38679   38697      +18     
Flag Coverage Δ
unit 5.85% <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/pie/pieRenderer.ts 0.00% <0.00%> (ø)

Copy link

argos-ci bot commented Jun 24, 2024

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) ⚠️ Changes detected (Review) 2 changed, 3 added Jul 17, 2024, 12:08 PM

@sidharthv96
Copy link
Member

@yari-dewalt can you add some tests in cypress/integration/rendering/pie.spec.ts which covers the long title case?

@yari-dewalt
Copy link
Collaborator Author

@sidharthv96 I went ahead and added some tests to cover the long title case, hopefully they're what you were looking for. I also added the default pieWidth in the totalWidth calculation in the renderer so there are not cut offs if there is no title / the title is not long enough

* develop: (87 commits)
  fix(class): #5624 Reset direction to default in classDiagram
  ci: upgrade to pnpm/action-setup@v4 to avoid CI failures
  chore(deps): update dependency eslint-plugin-unicorn to v54
  chore(deps): update dependency eslint-plugin-jsdoc to v48.5.0
  chore(deps): update all patch dependencies
  Sync docs folder
  Add Doctave to .cspell libraries
  Adds Doctave to list of integrations
  chore: update browsers list
  chore: Cleanup tsconfig
  chore: Fix lint
  Apply suggestions from code review
  chore: Remove extra words from cspell
  chore: Log granular rebuild times
  fix: Message wrap
  fix: Message wrap
  chore: Use `??` instead of `||`
  chore: Organise imports
  chore: Remove unused variables
  chore: Remove cross-env from eslint
  ...
@sidharthv96
Copy link
Member

Good set of tests @yari-dewalt!
Amazing work 🚀

@yari-dewalt yari-dewalt closed this by deleting the head repository Aug 21, 2024
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.

Pie chart : Title not visible if it is long
2 participants