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

FIXES: fix lint issues Fixes: #1106

Closed
wants to merge 6 commits into from
Closed

FIXES: fix lint issues Fixes: #1106

wants to merge 6 commits into from

Conversation

Anshgrover23
Copy link

fixes: #1038

Description

=> There were over 129 lint issues which i have fixes of type indentation errors,
FIXES: fix lint issues Fixes:

/claim #1038

Screenshots

Screenshot (67)

@alexanmtz
Copy link
Member

Hey @Anshgrover23 , thanks for the PR and great progress on this one, but you still need to set up the right ECMA script version to avoid errors on conditional chaining (variable1?.variable2)

There are still indentation errors to fix and unused variables to get all issues resolved, so please check those.

And we need to figure out why the tests are failing.

Copy link
Member

@alexanmtz alexanmtz left a comment

Choose a reason for hiding this comment

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

@Anshgrover23 please check these unneeded dependencies added

package.json Outdated Show resolved Hide resolved
@Anshgrover23 Anshgrover23 requested a review from alexanmtz June 15, 2024 13:21
Copy link
Member

@alexanmtz alexanmtz left a comment

Choose a reason for hiding this comment

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

The build is breaking and running the application I can see some errors on wrong imports, these should fix some but I can see there's still many errors, this is what I can see now. So let's fix these and continue to test.

frontend/src/components/profile/settings.js Outdated Show resolved Hide resolved
frontend/src/components/profile/payments.js Outdated Show resolved Hide resolved
frontend/src/index.js Outdated Show resolved Hide resolved
Copy link
Member

@alexanmtz alexanmtz left a comment

Choose a reason for hiding this comment

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

We have still these issues to address, but in general if you use it the lint-fix option, I would like to change to no semicolons in the end of the statement.

and some inconsistencies on the solution, sometimes you remove unused variables and others you comment to ignore the next line, other example you just remove, so it's better to have one solution, like remove those

return done(err)
};
// console.log('err', err)
// return done(err)
Copy link
Member

Choose a reason for hiding this comment

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

you're commenting here a line that can affect the tests besides the console, please don't do so.

You can remove the console and keep the line with the return

orderMail.expiredOrders(order)
return;
Copy link
Member

Choose a reason for hiding this comment

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

you removed a return statement needed here

@@ -112,7 +112,7 @@ const fetchTransferSuccess = (data) => {

const fetchTransferFailed = (error) => {
return {
type: FETCH_TRANSFER,
type: FETCH_TRANSFER_FAILED,
Copy link
Member

Choose a reason for hiding this comment

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

great catch 💯

Comment on lines +39 to +49
this.setState({ error: { ...this.state.error, fullname: true } });
} else {
this.setState({ error: { ...this.state.error, fullname: false } });
}
}

onChangeEmail (ev) {
onChangeEmail(ev) {
if (ev.target.value.length < 1) {
this.setState({ error: { email: true } })
}
else {
this.setState({ error: { email: false } })
this.setState({ error: { ...this.state.error, email: true } });
} else {
this.setState({ error: { ...this.state.error, email: false } });
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to keep the current state here because it's a object with nested keys

Comment on lines +47 to +50
if(user) {1+1}
if(hideExtra) {1+1}
if(size) {1+1}
if(classes) {1+1}
Copy link
Member

Choose a reason for hiding this comment

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

What is this?

componentWillMount () {
const token = this.props.match.params.token
const referer = Auth.getReferer()
componentDidMount() {
Copy link
Member

Choose a reason for hiding this comment

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

you changed the method here

@alexanmtz
Copy link
Member

Hey @Anshgrover23 , we just merged the #1107

So we will work from there and I will create a new issue to remove the warnings so I will close this one.

@alexanmtz alexanmtz closed this Jun 21, 2024
@Anshgrover23
Copy link
Author

Anshgrover23 commented Jun 21, 2024 via email

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.

Fix Lint issues
2 participants