Skip to content

feat: debug state flag added to chart status#834

Merged
nickofthyme merged 9 commits intoelastic:masterfrom
nickofthyme:debug-stats-flag
Sep 30, 2020
Merged

feat: debug state flag added to chart status#834
nickofthyme merged 9 commits intoelastic:masterfrom
nickofthyme:debug-stats-flag

Conversation

@nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Sep 24, 2020

Summary

Related to #794

Adds debugState prop to Settings component to render textual representation of chart.

see http://localhost:9001/?path=/story/debug-options--debug-state

image

Checklist

  • Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
  • This was checked for cross-browser compatibility
  • Proper documentation or storybook story was added for features that require explanation or tutorials
  • Unit tests were updated or added to match the most common scenarios

@nickofthyme nickofthyme added enhancement New feature or request :vislib Relating to vislib replacement labels Sep 24, 2020
Copy link
Collaborator

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

The code looks good and works perfectly. I've left few comments on the data structure and API that should be discussed

@nickofthyme nickofthyme changed the title Add debug state flag to chart status feat: debug state flag added to chart status Sep 29, 2020
@nickofthyme nickofthyme marked this pull request as ready for review September 29, 2020 17:03
Copy link
Collaborator

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

LGTM, I've added some minor comments.
Please remember to remove the elastic-charts-22.0.0.tgz file

Comment on lines +11 to +12
// Warning: (ae-forgotten-export) The symbol "AccessorObjectKey" needs to be exported by the entry point index.d.ts
// Warning: (ae-forgotten-export) The symbol "AccessorArrayIndex" needs to be exported by the entry point index.d.ts
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should fix these and export also AccessorObjectKey and AccessorArrayIndex

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comment on lines +115 to +132
const label = displayValue?.text;
if (!buckets.has(key)) {
const name = seriesNameMap.get(key) ?? '';
buckets.set(key, {
key,
name,
color,
bars: [],
labels: [],
visible: hasVisibleStyle(rect) || hasVisibleStyle(rectBorder),
});
}

buckets.get(key)!.bars.push({ x, y, mark });

if (label) {
buckets.get(key)!.labels.push(label);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of using the non-null operator you can do something like

const bucket = buckets.get(key) || {
          key,
          name,
          color,
          bars: [],
          labels: [],
          visible: hasVisibleStyle(rect) || hasVisibleStyle(rectBorder),
        }

update the bucket with the values and then save it back to the map

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nickofthyme nickofthyme merged commit 83919ff into elastic:master Sep 30, 2020
@nickofthyme nickofthyme deleted the debug-stats-flag branch September 30, 2020 21:39
markov00 pushed a commit that referenced this pull request Sep 30, 2020
# [23.0.0](v22.0.0...v23.0.0) (2020-09-30)

### Bug Fixes

* render continuous line/area between non-adjacent points ([#833](#833)) ([9f9892b](9f9892b)), closes [#825](#825)

### Features

* debug state flag added to chart status ([#834](#834)) ([83919ff](83919ff))
* expose datum as part of GeometryValue ([#822](#822)) ([dcd7077](dcd7077))

### BREAKING CHANGES

* when rendering non-stacked line/area charts with a continuous x scale and no fit function,
the line/area between non-consecutive data points will be rendered as a continuous line/area without adding an uncertain dashed line/ semi-transparent area that connects the two, non-adjacent, points.
@markov00
Copy link
Collaborator

🎉 This PR is included in version 23.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Sep 30, 2020
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [23.0.0](elastic/elastic-charts@v22.0.0...v23.0.0) (2020-09-30)

### Bug Fixes

* render continuous line/area between non-adjacent points ([opensearch-project#833](elastic/elastic-charts#833)) ([5222c40](elastic/elastic-charts@5222c40)), closes [opensearch-project#825](elastic/elastic-charts#825)

### Features

* debug state flag added to chart status ([opensearch-project#834](elastic/elastic-charts#834)) ([f3aba25](elastic/elastic-charts@f3aba25))
* expose datum as part of GeometryValue ([opensearch-project#822](elastic/elastic-charts#822)) ([e582bd6](elastic/elastic-charts@e582bd6))

### BREAKING CHANGES

* when rendering non-stacked line/area charts with a continuous x scale and no fit function,
the line/area between non-consecutive data points will be rendered as a continuous line/area without adding an uncertain dashed line/ semi-transparent area that connects the two, non-adjacent, points.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request released Issue released publicly :vislib Relating to vislib replacement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants