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

[FE] Fix eslint and prettier errors #117

Merged
merged 4 commits into from
Nov 24, 2020

Conversation

whotake
Copy link
Contributor

@whotake whotake commented Nov 11, 2020

This PR aims to fix existing Eslint and Prettier errors in the project

  • Bump Eslint packages
  • Fix existing Eslint and Prettier errors on an existing configuration
  • Fix lint, lint:fix and commit hook ts check
  • Add unique fields to data to follow react/no-array-index-key rule

fetchBrokerMetrics,
} from 'redux/actions';
import Brokers from './Brokers';
import { fetchBrokers, fetchBrokerMetrics } from 'redux/actions';
Copy link
Member

Choose a reason for hiding this comment

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

why is it better?

{activeControllers}
</Indicator>
<Indicator label="Total Brokers">{brokerCount}</Indicator>
<Indicator label="Active Controllers">{activeControllers}</Indicator>
Copy link
Member

Choose a reason for hiding this comment

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

why is it better?

params: { clusterName },
},
}: OwnProps
) => ({
Copy link
Member

Choose a reason for hiding this comment

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

why is it better? Somewhere you collapse objects, and somewhere you don't. What logic should we follow? only the length of the line?

className="title is-6 has-text-overflow-ellipsis"
title={name}
>
<div className="title is-6 has-text-overflow-ellipsis" title={name}>
Copy link
Member

Choose a reason for hiding this comment

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

why is it better?

{chunk.map((cluster, idx) => (
<ClusterWidget {...cluster} key={`dashboard-cluster-list-item-key-${idx}`}/>
{clusterList.map((chunkItem) => (
<div className="columns" key={v4()}>
Copy link
Member

Choose a reason for hiding this comment

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

why is it better? I believe v4 assigns new key for each <div> every time, so it is useless. Moreover, it can cause uncontrolled re-renderings of elements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it, replaced key value with unique fields. Unfortunately, not all of the data had it, so I added it to them.

kafka-ui-react-app/src/components/Topics/List/List.tsx Outdated Show resolved Hide resolved
@workshur
Copy link
Member

To avoid any confusions I would suggest to make config as strict as it possible

import {
getIsClusterListFetched,
getClusterList,
} from 'redux/reducers/clusters/selectors';
Copy link

Choose a reason for hiding this comment

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

Indeed, as @workshur mentioned it is not clear why we stretch the code over several lines here, while we collapse similar imports in other places.
This comment is just to draw your attention to the inconsistency.

@soffest
Copy link

soffest commented Nov 11, 2020

Totally agree with @workshur about confusing line breaking strategy and uniq keys for list items. Everything else LGTM.

@whotake
Copy link
Contributor Author

whotake commented Nov 13, 2020

@workshur @soffest All line breaking changes done by prettier based on max line width. Prettier configuration already existed in the project, but not all files followed it.

@whotake whotake changed the title [FE] Fix eslint errors [FE] Fix eslint and prettier errors Nov 13, 2020
…int-errors

# Conflicts:
#	kafka-ui-react-app/package-lock.json
#	kafka-ui-react-app/src/components/Brokers/Brokers.tsx
#	kafka-ui-react-app/src/components/Brokers/BrokersContainer.ts
#	kafka-ui-react-app/src/components/Dashboard/ClustersWidget/ClusterWidget.tsx
#	kafka-ui-react-app/src/components/Dashboard/ClustersWidget/ClustersWidget.tsx
#	kafka-ui-react-app/src/components/Topics/Details/DetailsContainer.ts
#	kafka-ui-react-app/src/components/Topics/Details/Messages/Messages.tsx
#	kafka-ui-react-app/src/components/Topics/List/ListItem.tsx
#	kafka-ui-react-app/src/components/Topics/New/NewContainer.ts
#	kafka-ui-react-app/src/components/Topics/TopicsContainer.ts
#	kafka-ui-react-app/src/redux/actions/actions.ts
#	kafka-ui-react-app/src/redux/api/clusters.ts
#	kafka-ui-react-app/src/redux/api/consumerGroups.ts
#	kafka-ui-react-app/src/redux/api/topics.ts
#	kafka-ui-react-app/src/redux/interfaces/broker.ts
#	kafka-ui-react-app/src/redux/interfaces/topic.ts
#	kafka-ui-react-app/src/redux/reducers/clusters/selectors.ts
#	kafka-ui-react-app/src/redux/reducers/topics/reducer.ts
@whotake whotake merged commit 3c4ccf9 into provectus:master Nov 24, 2020
@whotake whotake deleted the enhancement/fix-eslint-errors branch November 24, 2020 10:25
javalover123 pushed a commit to javalover123/kafka-ui that referenced this pull request Dec 7, 2022
…errors

[FE] Fix eslint and prettier errors
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

3 participants