Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Fix 2 issues on Dashboard #305

Merged

Conversation

yizheliu-amazon
Copy link
Contributor

Issue #, if available:

Description of changes:
Issue 1: Show live chart even when live anomaly data is empty

Before
Screen Shot 2020-09-04 at 9 48 46 AM

After
Screen Shot 2020-09-04 at 9 48 16 AM

Issue 2: Fix full screen issue for Dashboard

Before
Screen Shot 2020-09-04 at 11 23 09 AM

After
Screen Shot 2020-09-04 at 11 22 24 AM

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[AD_DOC_FIELDS.DETECTOR_NAME]: !isEmpty(liveAnomalyData) ? '' : null,
[AD_DOC_FIELDS.DETECTOR_NAME]: !isEmpty(liveAnomalyData)
? ''
: SPACE_STR,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only change for fix on the live chart issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks weird to set detector name as '' or ' ' , can you explain why we need an empty detector name or space as detector name ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When there exists liveAnomalyData, using '' the empty string makes sure those placeholder data won't be shown in legend because its name is ''; When liveAnomalyData does not exist, using space string as detector name can make sure data is plotted in the chart, and because in the case of no liveAnomalyData, legend is hidden, then we are able to see the live chart shown, even without actual anomaly data. Actually the SPACE_STR can be anything, since legend is hidden in that case.

</Fragment>
)}
</Fragment>
<div style={{ height: '1200px' }}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only change for full screen issue. The root cause is that not all parent divs have specified 100% height or explicit px for height. More details can be found here: https://stackoverflow.com/a/31728799

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I manually fix the height for dashboard to 1200 px, which looks appropriate on UI, without too much empty space at the bottom

Without anomaly
Screen Shot 2020-09-04 at 11 33 22 AM

With anomaly
Screen Shot 2020-09-04 at 11 34 48 AM

@ohltyler
Copy link
Contributor

ohltyler commented Sep 4, 2020

Can you confirm if all tests and/or test snapshots are passing?

@yizheliu-amazon
Copy link
Contributor Author

Can you confirm if all tests and/or test snapshots are passing?

Sure. Run it on my local workspace:

Test Suites: 1 skipped, 47 passed, 47 of 48 total
Tests:       4 skipped, 220 passed, 224 total
Snapshots:   35 passed, 35 total
Time:        44.994s
Ran all test suites.
Done in 49.45s

Copy link
Contributor

@ohltyler ohltyler left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes!

@yizheliu-amazon yizheliu-amazon merged commit 73ce1c6 into opendistro-for-elasticsearch:master Sep 4, 2020
yizheliu-amazon added a commit that referenced this pull request Sep 4, 2020
* Show live chart even when live anomaly data is empty

* Fix full screen issue for Dashboard
@yizheliu-amazon yizheliu-amazon added the bug Something isn't working label Sep 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants