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: debug state flag added to chart status #834

Merged
merged 9 commits into from
Sep 30, 2020

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
Member

@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

src/components/chart_status.tsx Show resolved Hide resolved
src/state/selectors/get_debug_state.ts Outdated Show resolved Hide resolved
src/state/selectors/get_debug_state.ts Outdated Show resolved Hide resolved
src/specs/settings.tsx Outdated Show resolved Hide resolved
@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
Member

@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
Member

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
Member

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
Member

🎉 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