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/5293_Allow_NaN_For_XYchart #5369

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

JingyuMarcelLee
Copy link
Contributor

📑 Summary

Allows NaN values for line and bar charts for XY chart.

    xychart-beta
      title "Sales Revenue"
      x-axis Months [jan, feb, mar, apr, may, jun, jul, aug, sep, oct, nov, dec]
      y-axis "Revenue (in $)" -2000 --> 11000
      bar [5000, NaN, 7500, 8200, 9500, 10500, NaN, 10200, 9200, 8500, 7000, 6000]
      line [5000, NaN, 7500, 8200, 9500, 10500, NaN, 10200, 9200, 8500, 7000, 6000]

Resolves #5293

📏 Design Decisions

For the bar chart, the bars are rendered with a height of 0 so the bar is not displayed when NaN values are passed in. For the line chart, the lines are broken where NaN values are passed in and are continued at the non-missing value. The mermaid code above will generate the diagram below.

📋 Tasks

Make sure you

Added NaN token to xychart.jison. Added logic in xychartRenderer.ts and linePlot.ts to parse NaN value and correctly display them on bar and line charts. Added test cases for xychart.jison
@github-actions github-actions bot added the Type: Bug / Error Something isn't working or is incorrect label Mar 7, 2024
Copy link

netlify bot commented Mar 7, 2024

Deploy Preview for mermaid-js failed.

Name Link
🔨 Latest commit c4f61ed
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/65fa75c21f4c2e00080a6ef4

Copy link

codecov bot commented Mar 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.67%. Comparing base (10fb856) to head (c4f61ed).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #5369   +/-   ##
========================================
  Coverage    44.67%   44.67%           
========================================
  Files           25       25           
  Lines         5341     5341           
  Branches        27       27           
========================================
  Hits          2386     2386           
  Misses        2954     2954           
  Partials         1        1           
Flag Coverage Δ
unit 44.67% <ø> (ø)

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

Comment on lines +120 to +121
| NAN COMMA commaSeparatedNumbers { $$ = [Number($NAN), ...$commaSeparatedNumbers] }
| NAN { $$ = [Number($NAN)] }
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this is a wrong place to represent NaN, because commaSeparatedNumbers represents the list, and does not necessarily have to deal with all the possible values. Adding more things apart from numbers will inflate the grammar. My suggestion is to combine NUMBER_WITH_DECIMAL and NAN into a separate rule called optionalValue, but my suggestion is optional, I guess 😃

@@ -57,6 +57,7 @@

"[" return 'SQUARE_BRACES_START'
"]" return 'SQUARE_BRACES_END'
"NaN" return 'NAN';
Copy link
Contributor

Choose a reason for hiding this comment

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

NaN is rather JS-originated, while many programming languages use their own keywords. Why not simply omitting the value?

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.

XYChart not allowing missing data points
2 participants