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/new configs 670 #3

Closed
wants to merge 13 commits into from
Closed

Conversation

rinku-kumar-psl
Copy link
Owner

Description

[Describe what this change achieves]

The features included are:

  • Legend
  • Mode
  • Interpolation
  • Line Width
  • Fill Opacity
  • Point Size

Issues Resolved

[List any issues this PR will resolve]
opensearch-project#697

Check List

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: rinku-kumar-psl <[email protected]>

# Conflicts:
#	dashboards-observability/public/components/visualizations/charts/lines/line_type.ts
Signed-off-by: rinku-kumar-psl <[email protected]>

# Conflicts:
#	dashboards-observability/public/components/visualizations/charts/lines/line_type.ts
Signed-off-by: rinku-kumar-psl <[email protected]>

# Conflicts:
#	dashboards-observability/public/components/visualizations/charts/lines/line.tsx
#	dashboards-observability/public/components/visualizations/charts/lines/line_type.ts
Signed-off-by: rinku-kumar-psl <[email protected]>

# Conflicts:
#	dashboards-observability/public/components/visualizations/charts/lines/line.tsx
#	dashboards-observability/public/components/visualizations/charts/lines/line_type.ts
Signed-off-by: rinku-kumar-psl <[email protected]>

# Conflicts:
#	dashboards-observability/public/components/visualizations/charts/lines/line.tsx
Signed-off-by: rinku-kumar-psl <[email protected]>
ShowLegend = 'show',
LegendPosition = 'v'
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a newline here

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done

export const LIVE_END_TIME ='now';

export enum DefaultChartStyles {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think, this should be a class or interface rather than an enum. You should define an interface first with all these properties and can create a simple object of that interface type which have these default values. Enum is usually used when similar items are there for considerations. Something like ChartStyles, which can be "Bar", "Tree", "Gause"... But this is some characteristics you are defining here, whose properties are not similar in nature.

Copy link
Owner Author

@rinku-kumar-psl rinku-kumar-psl May 23, 2022

Choose a reason for hiding this comment

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

Thanks, @spattnaik
changes are done as per your suggestion.

};
return (
<>
<DimensionComponent key={`viz-series-${index}`} {...params} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally, index should not be used as a key unless we have no other choices. Are there any other parameter we can use here? Is the Title unique?

Copy link
Owner Author

Choose a reason for hiding this comment

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

As per our discussion, kept as it is.

const currentSchemas = useMemo(() => {
if (!vizState?.style || vizState?.style === "lines") {
return schemas.filter((schema: IConfigPanelOptionSection) => schema.mapTo !== 'pointSize');
} else if (vizState?.style === "bar") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You do not need else blocks here. All blocks can be converted to if blocks. You will never have an else condition, as the function is going to terminate with the return statement.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done.

return schemas.filter((schema: IConfigPanelOptionSection) => !["interpolation", "pointSize"].includes(schema.mapTo));
} else if (vizState?.style === "markers") {
return schemas.filter((schema: IConfigPanelOptionSection) => ["style", "pointSize"].includes(schema.mapTo));
} else return schemas.filter((schema: IConfigPanelOptionSection) => schema.mapTo !== 'interpolation');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be a simple return statement without any else.

}
return (
<>
<DimensionComponent key={`viz-series-${index}`} {...params} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to an earlier comment, index should not ideally be used as the key.

Copy link
Owner Author

Choose a reason for hiding this comment

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

done

const [, r, g, b] = rgbElements.map((color) => parseInt(color, 16));
const rgbaFormat = `rgba(${r},${g},${b},${opacity})`;
return rgbaFormat;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add newline

Copy link
Owner Author

Choose a reason for hiding this comment

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

done

const showLegend = dataConfig?.legend?.showLegend && dataConfig.legend.showLegend !== ShowLegend ? false : true;
const legendPosition = dataConfig?.legend?.position || LegendPosition;
const markerSize = dataConfig?.chartStyles?.pointSize || MarkerSize;
const fillOpacity = dataConfig?.chartStyles?.fillOpacity !== undefined ? dataConfig?.chartStyles?.fillOpacity / 200 : FillOpacity / 200;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Avoid using numbers directly. Define what is 200 in a constant and then use it here.

Copy link
Owner Author

Choose a reason for hiding this comment

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

done

Signed-off-by: rinku-kumar-psl <[email protected]>
@rinku-kumar-psl
Copy link
Owner Author

Will raise external PR.

@rinku-kumar-psl rinku-kumar-psl deleted the feature/new-configs-670 branch May 25, 2022 09:04
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