Feature/metrics chart #2
Conversation
SivaprasadAluri
force-pushed
the
feature/metrics_chart
branch
from
cc371a2 to
469b1e4
Compare
| let listItem = { ...list[name][index] }; | ||
| listItem = { | ||
| ...listItem, | ||
| [field]: value, | ||
| }; |
There was a problem hiding this comment.
These two statements can be combined
|
|
||
| const updateList = (value: string, index: number, name: string, field: string) => { | ||
| let list = { ...configList }; | ||
| let listItem = { ...list[name][index] }; |
There was a problem hiding this comment.
What is the type of data present in the list[name][index]?
| ]); | ||
|
|
||
| const updateList = (value: string, index: number, name: string, field: string) => { | ||
| let list = { ...configList }; |
There was a problem hiding this comment.
Why not const? I do not see you updating the list object later in the code.
There was a problem hiding this comment.
I also do not see an objective to create a new variable from configlist. Everywhere in this method, you are creating a copy of list variable using spread operator. You could have done the same using configlist.
| }; | ||
|
|
||
| const updateListWithAgg = (value: string[], index: number, name: string, field: string) => { | ||
| const list = { ...configList }; |
There was a problem hiding this comment.
Same as the comment above. What is the requirement of creating this new variable?
| ...list[name].slice(0, index), | ||
| { | ||
| ...listItem, | ||
| [field]: value.map((val) => val), |
There was a problem hiding this comment.
How is [field]: value.map((val) => val) different from [field]: value? In case you need to create a copy, you could have used a spread operator instead. map function executes callback functions for each item, which is significantly slower than just copying elements.
| const unselectedFields = fieldOptionList.filter((field) => !selectedFields[field.label]); | ||
| return unselectedFields.filter((field) => numericalTypes.includes(field.type)); |
There was a problem hiding this comment.
Could have been
return fieldOptionList
.filter((field) => !selectedFields[field.label])
.filter((field) => numericalTypes.includes(field.type))| const getCommonUI = (lists, sectionName: string) => | ||
| lists && | ||
| lists.map((singleField, index: number) => ( | ||
| <> |
There was a problem hiding this comment.
each map item should have a key. See, your map item is not the div, but the fragment itself.
| @@ -3,7 +3,7 @@ | |||
| * SPDX-License-Identifier: Apache-2.0 | |||
| */ | |||
|
|
|||
| import React, { useState } from 'react'; | |||
| import React, { useEffect, useState } from 'react'; | |||
There was a problem hiding this comment.
Why do you need useEffect here? I do not see any changes below. This is the only change github is showing.
| (dataConfig?.colorTheme?.length > 0 && | ||
| dataConfig.colorTheme.find((colorSelected) => colorSelected.name.name === field)?.color) || | ||
| '#000'; | ||
| const fontSize = dataConfig?.fontSize?.fontSize ? dataConfig?.fontSize?.fontSize : 48; |
There was a problem hiding this comment.
dataConfig?.fontSize?.fontSize question marks not required in the if block. If the condition is satisfied, the fontsize exists already.
| style={{ color: getSelectedColorTheme(metric.label, index) }} | ||
| key={index} | ||
| > | ||
| {metric.aggregation.length !== 0 && { metricLabel } && |
There was a problem hiding this comment.
Why do you need brackets around { metricLabel }? Am I missing anything?
| .filter((field) => !selectedFields[field.label]) | ||
| .filter((field) => numericalTypes.includes(field.type)); |
There was a problem hiding this comment.
Instead of looping over the items multiple times, is it possible to do the check once? Something like:
.filter((field) => !selectedFields[field.label] && numericalTypes.includes(field.type)). Let me know.
| } | ||
| > | ||
| <EuiComboBox | ||
| aria-label="Accessible screen reader label" |
There was a problem hiding this comment.
Please mention proper label. Accessible screen reader label is a generic text (probably provided by the component vendor).
| placeholder="Custom label" | ||
| value={singleField.custom_label} | ||
| onChange={(e) => updateList(e.target.value, index, sectionName, 'custom_label')} | ||
| aria-label="Use aria labels when no actual label is in use" |
There was a problem hiding this comment.
aria-label is not proper here as well. Read about what they are.
Signed-off-by: SivaprasadAluri <[email protected]>
Signed-off-by: SivaprasadAluri <[email protected]>
Signed-off-by: SivaprasadAluri <[email protected]>
Signed-off-by: SivaprasadAluri <[email protected]>
Signed-off-by: SivaprasadAluri <[email protected]>
Signed-off-by: SivaprasadAluri <[email protected]>
Signed-off-by: SivaprasadAluri <[email protected]>
Signed-off-by: SivaprasadAluri <[email protected]>
Signed-off-by: SivaprasadAluri <[email protected]>
SivaprasadAluri
force-pushed
the
feature/metrics_chart
branch
from
f6ff0c1 to
6645377
Compare
Description
Added Metrics Charts Styles Panel
Issues Resolved
opensearch-project#872