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: improved domain error handling #933

Merged

Conversation

nickofthyme
Copy link
Collaborator

Summary

Fixes #932

Improves domain errors to fallback to logical defaults.

Use computed minInterval when custom minInterval is invalid.

Use consistent rendering for custom domain. Currently, when min and max are provided that show no data, the charts is rendered. However, when setting a custom min domain that is greater than the computed max, no data was rendered. Now the chart will display with the domain set to [min, min], such as...

image

Similarly, when the custom max is less than the computed min, no data was being rendered. Now the domain will default to [max, max], such as...

Image 2020-12-02 at 2 37 39 PM

All previous errors replaced with Logger.warn logs.

Checklist

  • 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 the :data Data/series/scales related issue label Dec 2, 2020
@nickofthyme nickofthyme requested a review from markov00 December 2, 2020 22:14
@codecov-io
Copy link

codecov-io commented Dec 2, 2020

Codecov Report

Merging #933 (3c7c2f8) into master (cb279f3) will increase coverage by 0.53%.
The diff coverage is 68.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #933      +/-   ##
==========================================
+ Coverage   70.11%   70.64%   +0.53%     
==========================================
  Files         340      357      +17     
  Lines       10979    11324     +345     
  Branches     2382     2431      +49     
==========================================
+ Hits         7698     8000     +302     
- Misses       3267     3304      +37     
- Partials       14       20       +6     
Flag Coverage Δ
unittests 70.11% <68.31%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
src/specs/settings.tsx 87.09% <ø> (ø)
..._types/xy_chart/renderer/canvas/primitives/line.ts 16.66% <9.09%> (-8.34%) ⬇️
..._types/xy_chart/renderer/canvas/primitives/path.ts 12.50% <25.00%> (+3.06%) ⬆️
..._types/xy_chart/state/selectors/on_click_caller.ts 55.55% <38.46%> (ø)
...pes/xy_chart/state/selectors/get_cursor_pointer.ts 69.56% <60.00%> (-4.12%) ⬇️
...art/state/selectors/get_projected_scaled_values.ts 94.11% <94.11%> (ø)
src/chart_types/xy_chart/domains/x_domain.ts 100.00% <100.00%> (ø)
src/chart_types/xy_chart/domains/y_domain.ts 95.78% <100.00%> (+0.04%) ⬆️
src/chart_types/xy_chart/state/chart_state.tsx 100.00% <100.00%> (ø)
src/chart_types/xy_chart/state/utils/utils.ts 96.17% <100.00%> (+0.03%) ⬆️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7198b5d...6662939. Read the comment docs.

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!

@nickofthyme nickofthyme merged commit 94534a5 into elastic:master Dec 3, 2020
@nickofthyme nickofthyme deleted the better-domain-error-handling branch December 3, 2020 17:52
markov00 pushed a commit that referenced this pull request Dec 4, 2020
# [24.3.0](v24.2.0...v24.3.0) (2020-12-04)

### Bug Fixes

* **highlighter:** show default highlighted radius with hidden dots ([#926](#926)) ([8b167a4](8b167a4)), closes [#679](#679)
* **xy_chart:** improve line joins rendering ([#920](#920)) ([ec8041a](ec8041a))
* point highlight based on geom position and transform ([#934](#934)) ([7198b5d](7198b5d))

### Features

* allow no results component, don't require series ([#936](#936)) ([4766c23](4766c23))
* improved domain error handling ([#933](#933)) ([94534a5](94534a5))
@markov00
Copy link
Member

markov00 commented Dec 4, 2020

🎉 This PR is included in version 24.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Dec 4, 2020
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [24.3.0](elastic/elastic-charts@v24.2.0...v24.3.0) (2020-12-04)

### Bug Fixes

* **highlighter:** show default highlighted radius with hidden dots ([opensearch-project#926](elastic/elastic-charts#926)) ([351c20c](elastic/elastic-charts@351c20c)), closes [opensearch-project#679](elastic/elastic-charts#679)
* **xy_chart:** improve line joins rendering ([opensearch-project#920](elastic/elastic-charts#920)) ([9a6771c](elastic/elastic-charts@9a6771c))
* point highlight based on geom position and transform ([opensearch-project#934](elastic/elastic-charts#934)) ([196ee8d](elastic/elastic-charts@196ee8d))

### Features

* allow no results component, don't require series ([opensearch-project#936](elastic/elastic-charts#936)) ([6be5c8b](elastic/elastic-charts@6be5c8b))
* improved domain error handling ([opensearch-project#933](elastic/elastic-charts#933)) ([f480054](elastic/elastic-charts@f480054))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:data Data/series/scales related issue released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should use fallback minInterval and not error the chart
3 participants