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

[Feat]: Dynamic columns in tables #3809

Merged
merged 10 commits into from
Oct 27, 2023

Conversation

Rajat-Dabade
Copy link
Contributor

Closes #3740

@Rajat-Dabade Rajat-Dabade self-assigned this Oct 27, 2023
@ankitnayan ankitnayan self-requested a review October 27, 2023 12:47
}

return (
<Typography className="DateComponent-container">{timeString}</Typography>
Copy link
Member

Choose a reason for hiding this comment

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

fix className

@YounixM
Copy link
Member

YounixM commented Oct 27, 2023

@Rajat-Dabade: Please add screenshots.

className="Dropdown-button"
onClick={(e): void => e.preventDefault()}
>
<Space>
Copy link
Member

Choose a reason for hiding this comment

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

Is Space required?


.dynamicColumnsTable-items {
display: flex;
width: 10.625rem;
Copy link
Member

Choose a reason for hiding this comment

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

Non responsive

index,
checked,
});
setColumnsData((prevColumns) => {
Copy link
Member

Choose a reason for hiding this comment

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

move to separate function

function DateComponent(
CreatedOrUpdateTime: string | number | Date,
): JSX.Element {
const time = new Date(CreatedOrUpdateTime);
Copy link
Member

Choose a reason for hiding this comment

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

handle invalid CreatedOrUpdateTime

try {
const storedData = localStorage.getItem(tablesource);
if (typeof storedData === 'string' && dynamicColumns) {
columnVisibilityData = JSON.parse(storedData);
Copy link
Member

Choose a reason for hiding this comment

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

will fail if storedData is not a valid JSON.


localStorage.setItem(tablesource, JSON.stringify(initialColumnVisibility));
} catch (error) {
console.error(error);
Copy link
Member

Choose a reason for hiding this comment

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

Are we handling the error silently or any UI feedback is required?

try {
const storedData = localStorage.getItem(tablesource);
if (typeof storedData === 'string' && dynamicColumns) {
const columnVisibilityData = JSON.parse(storedData);
Copy link
Member

Choose a reason for hiding this comment

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

Same comments as above.

import { getLabelRenderingValue } from './utils';

function LabelColumn({ labels, value, color }: LabelColumnProps): JSX.Element {
const newLabels = labels.length > 3 ? labels.slice(0, 3) : labels;
Copy link
Member

@YounixM YounixM Oct 27, 2023

Choose a reason for hiding this comment

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

add safety checks for labels if default initialization is not done.

const remainingLabels = labels.length > 3 ? labels.slice(3) : [];

return (
<div className="LabelColumn">
Copy link
Member

Choose a reason for hiding this comment

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

className should follow camelCase /hyphenated lowercase /BEM convention

<div>
{labels.map(
(label: string): JSX.Element => {
const tooltipTitle =
Copy link
Member

@YounixM YounixM Oct 27, 2023

Choose a reason for hiding this comment

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

can be moved to a separate function for better readability.

width: 80,
key: DynamicColumnsKey.UpdatedBy,
align: 'center',
render: (value): JSX.Element => <div>{value}</div>,
Copy link
Member

Choose a reason for hiding this comment

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

All values are by default rendered, render function isn't required

dataIndex: 'createdBy',
width: 30,
key: DynamicColumnsKey.CreatedBy,
render: (value): JSX.Element => <div>{value}</div>,
Copy link
Member

Choose a reason for hiding this comment

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

same comment as above.

@ankitnayan ankitnayan merged commit fc49833 into SigNoz:develop Oct 27, 2023
11 checks passed
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.

Add createdBy, createdAt, updatedBy and updatedAt fields in the alert list page
3 participants