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

[Frontend] Some console errors fixes on settings page #5080

Merged
merged 2 commits into from
Dec 5, 2023

Conversation

lndrtrbn
Copy link
Member

@lndrtrbn lndrtrbn commented Dec 4, 2023

Proposed changes

  • Diverse small fixes to clear a bit errors in dev console

Related issues

  • No related issues, just QoL for developers

Checklist

  • I consider the submitted work as finished
  • I tested the code for its functionality
  • I wrote test cases for the relevant uses case
  • I added/update the relevant documentation (either on github or on notion)
  • Where necessary I refactored code to improve the overall quality

Comment on lines -68 to +71
{...fieldToTextField(props)}
{...otherProps}
value={value ?? ''}
Copy link
Member Author

Choose a reason for hiding this comment

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

Reason: value can be null. But an input should not be initialized with null or undefined value, instead use an empty string. By so, React can properly track changes with the input.

value={field.value}
value={field.value ?? ''}
Copy link
Member Author

Choose a reason for hiding this comment

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

Reason: value can be null. But an input should not be initialized with null or undefined value, instead use an empty string. By so, React can properly track changes with the input.

Comment on lines -82 to +85
{...fieldToSelect(props)}
{...otherProps}
value={value ?? ''}
Copy link
Member Author

Choose a reason for hiding this comment

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

Reason: value can be null. But an input should not be initialized with null or undefined value, instead use an empty string. By so, React can properly track changes with the input.

Comment on lines -45 to +48
{...fieldToTextField(props)}
{...otherProps}
value={value ?? ''}
Copy link
Member Author

Choose a reason for hiding this comment

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

Reason: value can be null. But an input should not be initialized with null or undefined value, instead use an empty string. By so, React can properly track changes with the input.

settings.platform_cluster.instances_number
`${settings.platform_cluster.instances_number}`
Copy link
Member Author

Choose a reason for hiding this comment

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

Reason: neutralLabel accepts a string and not a number

return <div key="none" />;
return <div key={i} />;
Copy link
Member Author

Choose a reason for hiding this comment

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

Reason: always set a unique key to elements inside a loop. By so, React can correctly track each individual element.

Copy link
Member

Choose a reason for hiding this comment

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

could you find something based on the data ? edge.node.id maybe ?

Using index is a last-resort strategy that can lead to issues (see https://robinpokorny.com/blog/index-as-a-key-is-an-anti-pattern/)

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, this return is called when the data inside the map is undefined, so I can't really use any stuff linked to the data

Copy link
Member

Choose a reason for hiding this comment

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

ok then

Comment on lines -200 to +227
<ListItem style={{ paddingLeft: 0 }}>
<ListItemIcon>
<Skeleton
animation="wave"
variant="circular"
width={30}
height={30}
/>
</ListItemIcon>
<ListItemText
primary={
<div>
{Object.values(dataColumns).map((value) => (
<div
key={value.label}
className={classes.bodyItem}
style={{ width: value.width }}
>
<Skeleton
animation="wave"
variant="rectangular"
width="90%"
height={20}
/>
</div>
))}
</div>
}
<ListItemIcon>
<Skeleton
animation="wave"
variant="circular"
width={30}
height={30}
/>
<ListItemIcon>
<KeyboardArrowRightOutlined />
</ListItemIcon>
</ListItem>
</ListItemIcon>
<ListItemText
primary={
<div>
{Object.values(dataColumns).map((value) => (
<div
key={value.label}
className={classes.bodyItem}
style={{ width: value.width }}
>
<Skeleton
animation="wave"
variant="rectangular"
width="90%"
height={20}
/>
</div>
))}
</div>
}
/>
Copy link
Member Author

Choose a reason for hiding this comment

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

Just removed the imbricated ListItem as it was not required.

Reason: li inside li was generated.

@lndrtrbn lndrtrbn marked this pull request as ready for review December 4, 2023 10:15
@lndrtrbn lndrtrbn changed the title some console errors fixes on settings page [Frontend] Some console errors fixes on settings page Dec 4, 2023
@lndrtrbn lndrtrbn merged commit 35a682f into master Dec 5, 2023
6 checks passed
@lndrtrbn lndrtrbn deleted the green/fix_front_console_errors branch December 5, 2023 09:09
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.

None yet

2 participants