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: Adjust piechart viewbox for mobile devices with small width #4288

Merged
merged 12 commits into from
Nov 28, 2023

Conversation

iwestlin
Copy link
Contributor

@iwestlin iwestlin commented Apr 10, 2023

Resolves #4082
Resolves #2073
Resolves #1862

https://mermaid.js.org/syntax/pie.html#example This example will cut off the legend part in small width devices like mobile.

After implementing this PR, the pie chart will be able to display legends on small width devices. Please see the included image below:

pie chart legend

As someone new to SVG, I've only spent a few hours working on this issue. The most challenging aspect was getting the legend text's width before attaching it to the real DOM. I found a solution on Stack Overflow that involved using the canvas element. However, I didn't see the canvas element used in the Mermaid-js codebase, so it's possible that it might not work in nodejs/server-side implementations.

I'm not entirely certain if I've completed everything correctly yet, and I haven't run any tests. I'm simply attempting to attract the attention of the maintainers.😅

packages/mermaid/src/diagrams/pie/pieRenderer.js Outdated Show resolved Hide resolved
packages/mermaid/src/diagrams/pie/pieRenderer.js Outdated Show resolved Hide resolved
packages/mermaid/src/diagrams/pie/pieRenderer.js Outdated Show resolved Hide resolved
iwestlin added a commit to iwestlin/mermaid that referenced this pull request Jun 26, 2023
@iwestlin
Copy link
Contributor Author

Hi, @sidharthv96

Thank you for taking the time to review my work. I have made the changes you suggested and pushed a new commit.

iwestlin added a commit to iwestlin/mermaid that referenced this pull request Jun 26, 2023
iwestlin added a commit to iwestlin/mermaid that referenced this pull request Jun 26, 2023
iwestlin added a commit to iwestlin/mermaid that referenced this pull request Jun 26, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2023

Codecov Report

Merging #4288 (005c998) into develop (25e9bb3) will increase coverage by 0.08%.
The diff coverage is 91.66%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4288      +/-   ##
===========================================
+ Coverage    79.30%   79.38%   +0.08%     
===========================================
  Files          165      165              
  Lines        13860    13851       -9     
  Branches       704      703       -1     
===========================================
+ Hits         10991    10995       +4     
+ Misses        2719     2706      -13     
  Partials       150      150              
Flag Coverage Δ
e2e 85.20% <90.90%> (+<0.01%) ⬆️
unit 43.03% <100.00%> (+0.12%) ⬆️

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

Files Coverage Δ
packages/mermaid/src/setupGraphViewbox.js 77.46% <100.00%> (+12.40%) ⬆️
packages/mermaid/src/diagrams/pie/pieRenderer.ts 98.30% <90.90%> (+0.09%) ⬆️

packages/mermaid/src/diagrams/pie/pieRenderer.js Outdated Show resolved Hide resolved
packages/mermaid/src/diagrams/pie/pieRenderer.js Outdated Show resolved Hide resolved
packages/mermaid/src/rendering-util/getTextWidth.js Outdated Show resolved Hide resolved
* develop: (596 commits)
  chore(deps): update all minor dependencies
  chore(deps): update all patch dependencies
  fix: flowchart image without text
  fix Types
  chore: Update pnpm-lock
  chore: Add tests for calculateDeltaAndAngle
  fix: mermaid-js#5064 Handle case when line has only one point
  reset the testTimeout to 5 seconds and change it directly in the test
  update testTimeout from 5 seconds to 10 seconds
  Update all patch dependencies
  fix broken link
  add latest blog post
  Update all minor dependencies
  Update all patch dependencies
  Fix docs
  Update packages/mermaid/src/docs/community/questions-and-suggestions.md
  Update packages/mermaid/src/diagrams/class/classRenderer-v2.ts
  update edge ids
  draw top actors with lines  first followed by messages
  Bump GitHub workflow actions to latest versions
  ...
Copy link

netlify bot commented Nov 28, 2023

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 005c998
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/6565d187b42bb30008beb4b4
😎 Deploy Preview https://deploy-preview-4288--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 28, 2023
@sidharthv96 sidharthv96 requested review from aloisklink and a team November 28, 2023 06:11
@sidharthv96 sidharthv96 added this to the v10 milestone Nov 28, 2023
Copy link
Member

@aloisklink aloisklink left a comment

Choose a reason for hiding this comment

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

Looks good, assuming we can get E2E tests passing. I think I've fixed them locally, so I'll push up a fix (see 005c998).

Something to add to the PR description, the pieConfig.useWidth config field is no longer used, but that's okay in my opinion since this config option wasn't documented anyway.

It looks like this PR also improves pie charts that currently have labels that overflow, so this fix is extra nice 🥳:

pie-chart-should-render-a-simple-pie-diagram-with-long-labels diff

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

mermaid-bot bot commented Nov 28, 2023

@iwestlin, 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.

sidharthv96 added a commit that referenced this pull request Dec 4, 2023
* develop: (47 commits)
  chore(deps): update all minor dependencies
  chore: Rename test
  test: Add unit test for generic classname and namespace
  Split type from generic class name
  Referenced the PmWiki's Cookbook recipe enabling MermaidJs schematics in wiki pages
  test(e2e): fix pie chart E2E tests for PR #4288
  Add dummy commit to trigger GH checks
  chore: Revert unnecessary export
  refactor: Remove unnecessary calculations
  chore: Fix computeWidth function
  chore: Cleanup setupGraphViewbox
  Update docs
  update mermaidAPI to cleanup the text before passing to getDiagramFromText
  Add test for subgraphs with title margins and edge labels
  Modify margin logic to avoid creating unnecessary space in subgraph
  review fixes
  add test
  remove unused variable
  fix: clean comments in text in getDiagramFromText API so flowchart works well
  chore(deps): update all minor dependencies
  ...
sidharthv96 added a commit that referenced this pull request Dec 6, 2023
* develop: (70 commits)
  build(deps-dev): bump vite from 4.4.9 to 4.4.12
  Changes to .prettierignore 1. Added 'demos/dev/**' to be ignored by Prettier. 2. Added '!/demos/dev/example.html' so that Prettier ensures no one changes the example.html in a way that doesn't obey the Prettier code formatting rules.
  build: use `tsx` instead of `ts-node-esm`
  chore: Downgrade node to 18.18.2
  fix: #5100 Add viewbox to sankey
  chore(deps): update all minor dependencies
  chore: Rename test
  test: Add unit test for generic classname and namespace
  fix: Check if parentCommit is provided
  Split type from generic class name
  Condition of Parent Id Without Merge Commit Added
  Referenced the PmWiki's Cookbook recipe enabling MermaidJs schematics in wiki pages
  test(e2e): fix pie chart E2E tests for PR #4288
  Add dummy commit to trigger GH checks
  chore: Revert unnecessary export
  refactor: Remove unnecessary calculations
  chore: Fix computeWidth function
  chore: Cleanup setupGraphViewbox
  Update docs
  update mermaidAPI to cleanup the text before passing to getDiagramFromText
  ...
sidharthv96 added a commit to NicolasNewman/mermaid that referenced this pull request Dec 6, 2023
* develop: (192 commits)
  build(deps-dev): bump vite from 4.4.9 to 4.4.12
  Changes to .prettierignore 1. Added 'demos/dev/**' to be ignored by Prettier. 2. Added '!/demos/dev/example.html' so that Prettier ensures no one changes the example.html in a way that doesn't obey the Prettier code formatting rules.
  build: use `tsx` instead of `ts-node-esm`
  chore: Downgrade node to 18.18.2
  fix: mermaid-js#5100 Add viewbox to sankey
  chore(deps): update all minor dependencies
  chore: Rename test
  test: Add unit test for generic classname and namespace
  fix: Check if parentCommit is provided
  Split type from generic class name
  Condition of Parent Id Without Merge Commit Added
  Referenced the PmWiki's Cookbook recipe enabling MermaidJs schematics in wiki pages
  test(e2e): fix pie chart E2E tests for PR mermaid-js#4288
  Add dummy commit to trigger GH checks
  chore: Revert unnecessary export
  refactor: Remove unnecessary calculations
  chore: Fix computeWidth function
  chore: Cleanup setupGraphViewbox
  Update docs
  update mermaidAPI to cleanup the text before passing to getDiagramFromText
  ...
sidharthv96 added a commit that referenced this pull request Dec 7, 2023
* next: (387 commits)
  build(deps-dev): bump vite from 4.4.9 to 4.4.12
  Changes to .prettierignore 1. Added 'demos/dev/**' to be ignored by Prettier. 2. Added '!/demos/dev/example.html' so that Prettier ensures no one changes the example.html in a way that doesn't obey the Prettier code formatting rules.
  build: use `tsx` instead of `ts-node-esm`
  chore: Downgrade node to 18.18.2
  fix: #5100 Add viewbox to sankey
  chore(deps): update all minor dependencies
  chore: Rename test
  test: Add unit test for generic classname and namespace
  fix: Check if parentCommit is provided
  Split type from generic class name
  Condition of Parent Id Without Merge Commit Added
  Referenced the PmWiki's Cookbook recipe enabling MermaidJs schematics in wiki pages
  test(e2e): fix pie chart E2E tests for PR #4288
  Add dummy commit to trigger GH checks
  chore: Revert unnecessary export
  refactor: Remove unnecessary calculations
  chore: Fix computeWidth function
  chore: Cleanup setupGraphViewbox
  Update docs
  update mermaidAPI to cleanup the text before passing to getDiagramFromText
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Graph: Pie Type: Bug / Error Something isn't working or is incorrect
Projects
None yet
6 participants