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

1270 enable eslint on all js files (partially fixed) #1331

Merged
merged 8 commits into from
Aug 31, 2022

Conversation

edwinjue
Copy link
Member

@edwinjue edwinjue commented Aug 29, 2022

Partially fixes #1270

  • Up to date with dev branch
  • Branch name follows guidelines
  • All PR Status checks are successful
  • Peer reviewed and approved

Any questions? See the getting started guide

@edwinjue edwinjue requested a review from nichhk August 29, 2022 22:34
@edwinjue edwinjue linked an issue Aug 29, 2022 that may be closed by this pull request
2 tasks
Copy link
Member

@nichhk nichhk left a comment

Choose a reason for hiding this comment

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

Thanks Edwin! A couple small comments.

}
})
}
const updateAll = value => requestTypes.map(type => {
Copy link
Member

Choose a reason for hiding this comment

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

Let's use forEach here since we don't need to create a new array

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds good, changes made

requestTypes: Object.keys(requestTypes).filter(req => req !== 'All' && requestTypes[req]),
};
}
// function* getMapFilters() {
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing this function isn't being used? In that case, we can just delete it. Commented-out code is more confusing than useful in most cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

agreed, commented code has been removed

@edwinjue edwinjue changed the title 1270 enable eslint on all js files 1270 enable eslint on all js files (partially fixed) Aug 30, 2022
@edwinjue edwinjue requested a review from nichhk August 30, 2022 08:16
Copy link
Member

@nichhk nichhk left a comment

Choose a reason for hiding this comment

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

Thanks Edwin! One small comment. Please resolve and then feel free to merge the PR.

}
return null;
});
function updateAll(value) {
Copy link
Member

Choose a reason for hiding this comment

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

while you're here (I know you didn't write this): nit: value -> isSelected

Copy link
Member Author

Choose a reason for hiding this comment

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

all set

…in client/components/main/Desktop/TypeSelector/index.js
@edwinjue edwinjue merged commit 25dd5fb into dev Aug 31, 2022
@edwinjue edwinjue deleted the 1270-enable-eslint-on-all-js-files branch August 31, 2022 15:17
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.

Enable ESLint on all js files
2 participants