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

Feature/metrics chart #1

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

SivaprasadAluri
Copy link
Owner

@SivaprasadAluri SivaprasadAluri commented Aug 22, 2022

Description

Added Metrics Chart Styles Panel

Issues Resolved

opensearch-project#872

Comment on lines +181 to +182
},
];

Choose a reason for hiding this comment

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

Please add a empty line below. You can also use Prettify to format this file.

id="discover.noResults.queryMayNotMatchTitle"
values={Object {}}
>
Your query may not match anything in the current time range, or there may not be any data at all in the currently selected time range. Try change time range, query filters or choose different time fields
Your query may not match anything in the current time range, or there may not be any data at all in
the currently selected time range. Try change time range, query filters or choose different time fields
</FormattedMessage>
</p>
</div>

Choose a reason for hiding this comment

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

We don't push this snapshot. Please revert this file to its past state.

@@ -43,6 +43,7 @@

::-webkit-scrollbar {
width: 10px;
height: 10px;

Choose a reason for hiding this comment

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

remove the height.

Comment on lines 67 to 68
initialIsOpen
id="configPanel__legend"

Choose a reason for hiding this comment

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

use htmlIdGenerator for id

Comment on lines +98 to +101
} else if (visualizations.vis.name === visChartTypes.Metrics) {
setConfigList({
metrics: [initialConfigEntry],
});

Choose a reason for hiding this comment

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

please take a pull of latest code, we are using switch case here in place of if-else

</EuiFlexItem>
</>
);
};

Choose a reason for hiding this comment

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

We have changed this file. Please update the config list according to valueOptions

id: 'fontsize',
name: 'Metric FontSize',
editor: ConfigLegend,
mapTo: 'FontSize',

Choose a reason for hiding this comment

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

use camel-case for mapTo value

Comment on lines +17 to +19
const dataConfigTab =
visualizations.data?.rawVizData?.metrics?.dataConfig &&
visualizations.data.rawVizData.metrics.dataConfig;

Choose a reason for hiding this comment

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

Take a pull of the latest code with valueOptions and check whether this still works or not.

Comment on lines 28 to 37
if (aggregate === 'COUNT') {
return data[label].length;
} else if (aggregate === 'AVERAGE') {
return meanBy(data[label]).toFixed(2);
} else if (aggregate === 'MAX') {
return (max(data[label]) as number).toFixed(2);
} else if (aggregate === 'MIN') {
return (min(data[label]) as number).toFixed(2);
} else if (aggregate === 'SUM') {
return sum(data[label]).toFixed(2);

Choose a reason for hiding this comment

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

use switch case here

Signed-off-by: SivaprasadAluri <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants