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

Address couples of feedback. Issue: #26, #36, #37, #38 #45

Conversation

yizheliu-amazon
Copy link
Contributor

Issue #, if available:
Address couples of feedback. Issue: #26, #36, #37, #38
Description of changes:

  1. change the detector link on dashboard detector list table should redirect to AD result page
  2. add Fullscreen of dashboard live chart
  3. Fix issue that: for a new domain which has no AD plugin running, dashboard will always show loading status as we can’t get detectors (detector index not found). We need to show empty dashboard page
  4. Decrease line height for subtitle of Distribution Sunburst Chart on Dashboard
  5. Increase margin Live Anomalies title to 20px, make all the text/wording is aligned/consistent with other components
  6. Filter boxes can use full-width, margin between filter boxes is too wide(decrease to 8px), we can make filter boxes take the all whole row. The goal is to make all boxes long enough to avoid stacked multi-selections as much as possible
  7. Use dynamic height for Detector&Feature list, its height is not necessary to be same as Distribution Sunburst chart.
  8. Decrease size of Distribution Sunburst chart to 400px/360px
  9. Change style of subtitle of Live Anomaly chart.
  10. Keep XL loading chart only.
  11. Couples of style changes required by UX designer.
  12. Change anomaly live chart in cases of 1) no anomaly result; 2) there exists anomaly data; 3) only anomaly grade 0 data like below

Screen Shot 2020-04-23 at 9 45 19 PM
Screen Shot 2020-04-23 at 10 42 38 PM
Screen Shot 2020-04-23 at 11 09 48 PM

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

>
<EuiFlexGroup
style={{ padding: '0px 10px', ...props.titleContainerStyles }}
style={{ padding: '0px 20px', ...props.titleContainerStyles }}
Copy link
Contributor

Choose a reason for hiding this comment

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

How about we change this padding as 0

style={{ padding: '0px', ...props.titleContainerStyles }}

And set the padding of EuiPanel as 20px, so we can align title and content body.

<EuiPanel
    style={{ padding: '20px', ...props.panelStyles }}
    className={props.contentPanelClassName}
  >

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing the padding of EuiPanel to 20px will make the EuiHorizontalRule looks disconnected to the Panel. I changed the style for Live chart, and below is the screenshot.
Screen Shot 2020-04-27 at 5 52 28 PM

This is not same as the UX mock up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, let's follow the UX mock up

@@ -105,7 +107,7 @@ const ContentPanel = (props: ContentPanelProps) => (
className={props.horizontalRuleClassName}
/>
)}
<div style={{ padding: '0px 10px', ...props.bodyStyles }}>
<div style={{ padding: '0px 20px', ...props.bodyStyles }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why increase left and right padding? Shouldn't we increase the top and down padding?

Copy link
Contributor Author

@yizheliu-amazon yizheliu-amazon Apr 28, 2020

Choose a reason for hiding this comment

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

As mentioned above, since we cannot increase EuiPanel padding, we have to increase the left/right padding of body to 20px.

Regarding the top/down padding, you are right, we should make them both 20px. I didn't increase them because:

  1. Top padding: I set top padding in AnomaliesLiveChart and AnomaliesDistributionChart: FlexGroup has default padding -12px, and EuiHorizontalRule has default top/bottom margin 8px, changing top padding of FlexGroup to 0px can make the distance between EuiHorizontalRule and FlexItems in FlexGroup to 20px

  2. Bottom padding: EuiPanel has default padding 16px, thus I keep it for the bottom padding.

To consolidate style changes for ContentPanel, I think I can make following changes so that all the style change can be configured in ContentPanel, and the distance between components and Panel edge or EuiHorizontalRule is 20px.

  <EuiPanel
    style={{
      paddingLeft: '0px',
      paddingRight: '0px',
      paddingTop: '20px',
      paddingBottom: '20px',
      ...props.panelStyles,
    }}
    className={props.contentPanelClassName}
  >
    <EuiFlexGroup
      style={{ padding: '0px 20px', ...props.titleContainerStyles }}
      justifyContent="spaceBetween"
      alignItems="center"
    >
    <div
      style={{
        paddingTop: '12px', // 12px here is because EuiHorizontalRule has bottom margin 8px
        paddingLeft: '20px',
        paddingRight: '20px',
        ...props.bodyStyles,
      }}
    >
      {props.children}
    </div>

Please let me know your thoughts on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok about it, we can tune if any other comments come in.

// `id` for placeholder data point introduced by `prepareVisualizedAnomalies` shows as legend,
// When there exists anomalies with anomaly grade > 0
// we make `id` to blank string to hide the legend of placeholder data point
id={!isEmpty(liveAnomalyData) ? '' : ' '}
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious: only blank string can hide legend ? Should we set a normal id rather than whitespace when the liveAnomalyData not empty?

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 very tricky.

If detectorName value for placeholder data is set as null, the id of BarSeries will be shown as legend for placeholder data.

Given empty liveAnomalyData, the legend of placeholder data will be hidden by changing showLegend to false, aka showLegend={!isEmpty(liveAnomalyData)}. Thus we don't need to worry about legend of placeholder data in case of empty liveAnomalyData.

Given non-empty liveAnomalyData, the legend of placeholder data is shown along with legend of liveAnomalyData. Legend of placeholder data is not needed now. Changing id of BarSeries to blank string can make legend of placeholder data disappear, and normal legend of liveAnomalyData is still there.

But we cannot always pass blank string {''}to id, as in the case of null detectorName + id as {''} , EuiChart will show No data to dsiplay, which UX wants to avoid. That's why I pass {' '} /{''} to id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After second thought, I changed to

[AD_DOC_FIELDS.DETECTOR_NAME]: !isEmpty(liveAnomalyData) ? '' : null,

and keep using

<BarSeries
id={'Detector Anomaly grade'}`

It can also work. will change to that implementation and post another commit.

from: 0,
size: 1,
search: '',
sortDirection: SORT_DIRECTION.DESC,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use ASC sort direction? name is just string, is it better to show alphabetically?

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 just used to check if there is any detector ever created. That is why size is 1. If no detector, we move to EmptyDashboard page. If yes, we navigate to the real Dashboard page. using ASC or DESC doesn't matter too much here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any smarter way of doing the check of existing detector is appreciated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, got it, that's ok. Maybe we can use _count in future.
https://stackoverflow.com/questions/25739888/counting-number-of-documents-using-elasticsearch

Probably _count is a bit faster since it doesn't have to execute a full query with ranking and result fetching and can simply return the size.

@yizheliu-amazon yizheliu-amazon merged commit e6f08ae into opendistro-for-elasticsearch:development Apr 29, 2020
@yizheliu-amazon yizheliu-amazon deleted the couple-feedback-fix branch April 29, 2020 16:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants