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

feat: Add point styling for quadrant chart #5173

Conversation

ilyes-ced
Copy link
Contributor

@ilyes-ced ilyes-ced commented Jan 2, 2024

📑 Summary

new_colors

addes ability to add point styling to quadrant charts

  • radius
  • color
  • stroke color
  • stroke width

Resolves #4995

📏 Design Decisions

quadrantChart
      title Reach and engagement of campaigns
      x-axis Low Reach --> High Reach
      y-axis Low Engagement --> High Engagement
      quadrant-1 We should expand
      quadrant-2 Need to promote
      quadrant-3 Re-evaluate
      quadrant-4 May be improved
      Campaign A: [0.9, 0.2] radius: 12
      Campaign B: [0.9, 0.4] radius: 10 color: #ff3300
      Campaign C: [0.9, 0.6] radius: 25 color: #00ff33 stroke_color: #10f0f0
      Campaign D: [0.9, 0.8] radius: 15 color: #ff33f0 stroke_color: #00ff0f stroke_width: 5px
      Campaign A11: [0.1, 0.8] radius: 15
      Campaign D11: [0.1, 0.6] color: #ff33f0
      Campaign E11: [0.1, 0.4] stroke_color: #fff000
      Campaign F11: [0.1, 0.2] stroke_width: 5px

adds ability to add those 4 parameters

❌ the problem

it can either accept one of the 4 paramaters or accept 1,2,3 or 4 params in this specific order
it would accept one of those 4

      radius: 2
      radius: 10 color: #198463 
      radius: 30 color: #ff3300 border_color: #10f0f0 
      radius: 15 color: #ff33f0 border_color: #00ff0f border_width: 5px

it wouldnt accept for example

color: #ff33f0 radius: 15 border_color: #00ff0f border_width: 5px

the reason

i am unfimiliar with jison so i did this to make accepting parmas optional in file packages/mermaid/src/diagrams/quadrant-chart/parser/quadrant.jison

points
  : text point_start point_x point_y {yy.addPoint($1, $3, $4);}
  | text point_start point_x point_y point_radius {yy.addPoint($1, $3, $4, $5);}
  | text point_start point_x point_y point_color  {yy.addPoint($1, $3, $4, "", $5);}
  | text point_start point_x point_y stroke_color {yy.addPoint($1, $3, $4, "", "", $5);}
  | text point_start point_x point_y stroke_width {yy.addPoint($1, $3, $4, "", "", "", $5);}
  | text point_start point_x point_y point_radius point_color {yy.addPoint($1, $3, $4, $5, $6);}
  | text point_start point_x point_y point_radius point_color stroke_color {yy.addPoint($1, $3, $4, $5, $6, $7);}
  | text point_start point_x point_y point_radius point_color stroke_color stroke_width {yy.addPoint($1, $3, $4, $5, $6, $7, $8);}
  ;

following this pattren to include all possible combination it would require 81 lines like this
and this is where i need advice on how to deal with this

📋 Tasks

Make sure you

  • 📖 have read the contribution guidelines
  • 💻 have added necessary unit/e2e tests. (will add more after finishing the pr)
  • (not yet will add them when the pr is complete) 📓 have added documentation. Make sure MERMAID_RELEASE_VERSION is used for all new features.
  • 🔖 targeted develop branch

Copy link

netlify bot commented Jan 2, 2024

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit eb4a6fd
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/661ded5a5b98000008bdd34c
😎 Deploy Preview https://deploy-preview-5173--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: Enhancement New feature or request label Jan 2, 2024
Copy link

codecov bot commented Jan 2, 2024

Codecov Report

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

Project coverage is 5.73%. Comparing base (8e95c4d) to head (eb4a6fd).
Report is 45 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #5173      +/-   ##
==========================================
- Coverage     5.74%   5.73%   -0.02%     
==========================================
  Files          277     277              
  Lines        41899   42002     +103     
  Branches       489     515      +26     
==========================================
  Hits          2407    2407              
- Misses       39492   39595     +103     
Flag Coverage Δ
e2e ?
unit 5.73% <0.00%> (-0.02%) ⬇️

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

Files Coverage Δ
...id/src/diagrams/quadrant-chart/quadrantRenderer.ts 0.00% <0.00%> (ø)
...kages/mermaid/src/diagrams/quadrant-chart/utils.ts 0.00% <0.00%> (ø)
...aid/src/diagrams/quadrant-chart/quadrantBuilder.ts 0.00% <0.00%> (ø)
.../mermaid/src/diagrams/quadrant-chart/quadrantDb.ts 0.00% <0.00%> (ø)

... and 8 files with indirect coverage changes

@ilyes-ced ilyes-ced changed the title add: point styling for quadrant chart (incomplete) add: point styling for quadrant chart (incomplete need advice) Jan 3, 2024
@ilyes-ced ilyes-ced changed the title add: point styling for quadrant chart (incomplete need advice) add: point styling for quadrant chart (incomplete, need advice) Jan 3, 2024
@nirname nirname requested a review from a team January 5, 2024 14:09
@sidharthv96 sidharthv96 marked this pull request as draft January 11, 2024 04:15
@sidharthv96
Copy link
Member

When adding support for styling, we should also allow the same style to be applied to multiple points easily. Flowchart uses classes to accomplish this.

https://mermaid.js.org/syntax/flowchart.html#styling-and-classes

@nirname nirname self-requested a review January 11, 2024 11:19
@ilyes-ced
Copy link
Contributor Author

changed it to take styles as a single string in the quadron.jison file and parse it in the quadrantDb.ts file

styling with classnames is yet to be implemented, working on it still

@ilyes-ced
Copy link
Contributor Author

i made styles optional and acceptable in random order
i also added class names and class definitions like so
i made them same as flowcharts and also class names styles take priority over the others same as flowcharts

quadrantChart
      title Reach and engagement of campaigns
      x-axis Low Reach --> High Reach
      y-axis Low Engagement --> High Engagement
      quadrant-1 We should expand
      quadrant-2 Need to promote
      quadrant-3 Re-evaluate
      quadrant-4 May be improved
      Campaign A: [0.9, 0.0] radius: 12
      Campaign B:::class1: [0.8, 0.1] color: #ff3300, radius: 10
      Campaign C: [0.7, 0.2] radius: 25, color: #00ff33, stroke-color: #10f0f0      
      Campaign D: [0.6, 0.3] radius: 15, stroke-color: #00ff0f, stroke-width: 5px ,color: #ff33f0       
      Campaign E:::class2: [0.5, 0.4]
      Campaign F:::class3: [0.4, 0.5] color: #0000ff
      classDef class1 color: #109060
      classDef class2 color: #908342, radius : 10, stroke-color: #310085, stroke-width: 10px
      classDef class3 color: #f00fff, radius : 10

new_colorss

❌ the problem:

i keep getting those pnpm test errors that i can't figure out where they are coming from
tests

@ilyes-ced ilyes-ced requested a review from sidharthv96 January 12, 2024 20:49
@ilyes-ced
Copy link
Contributor Author

ilyes-ced commented Jan 12, 2024

there also a few places that i need your advice as for what to do
comments here:

  1. mermaid/packages/mermaid/src/diagrams/quadrant-chart/quadrantDb.ts:72
  2. mermaid/packages/mermaid/src/diagrams/quadrant-chart/quadrantDb.ts:106
  3. mermaid/packages/mermaid/src/diagrams/quadrant-chart/quadrantDb.ts:109

@ilyes-ced
Copy link
Contributor Author

ilyes-ced commented Feb 7, 2024

various fixes

  • styles now are parsed in jison and outputed like [style: value, style2: value2 .... ] then it is accepted by function parseStyles in mermaid/src/diagrams/quadrant-chart/quadrantDb.ts
  • unit tests for parseStyles created in the file mermaid/packages/mermaid/src/diagrams/quadrant-chart/quadrantDb.spec.ts (not sure if creating a new file is the correct way i didnt find any other unit tests but the jison ones)
  • tests now work normally
  • throws error when given an unknown stlye
  • jison throws error automatically when classDef is missing a name or the styles

@ilyes-ced ilyes-ced requested a review from sidharthv96 February 7, 2024 00:26
@ilyes-ced ilyes-ced marked this pull request as ready for review February 7, 2024 00:26
@sidharthv96 sidharthv96 changed the title add: point styling for quadrant chart (complete) feat: Add point styling for quadrant chart Mar 6, 2024
@sidharthv96 sidharthv96 requested review from aloisklink and a team March 6, 2024 04:35
@ilyes-ced ilyes-ced requested a review from sidharthv96 March 24, 2024 19:00
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.

@ilyes-ced we forgot a major part, the docs.
This should be ready to go once the docs are added.

@ilyes-ced
Copy link
Contributor Author

not sure if what i added is enough

@ilyes-ced ilyes-ced requested a review from sidharthv96 March 26, 2024 23:13
@sidharthv96
Copy link
Member

@ilyes-ced the docs were added to the wrong file, so they were deleted when docs:build ran.
I've added them back, and made some changes on the precedence of styles.
Now the order is Direct > Classes > Themes

@sidharthv96 sidharthv96 merged commit 3809732 into mermaid-js:develop Apr 16, 2024
18 of 19 checks passed
Copy link

mermaid-bot bot commented Apr 16, 2024

@ilyes-ced, 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.

@ilyes-ced ilyes-ced deleted the feature/add-point-styling-quadrant-to-charts branch April 16, 2024 20:32
@ilyes-ced ilyes-ced restored the feature/add-point-styling-quadrant-to-charts branch April 16, 2024 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Point Styling in Quadrant Chart
2 participants