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

Feature/cleanup eslint errors #295

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

seanwash
Copy link
Collaborator

Description

What does this PR do?

It cleans up all of the outstanding eslint errors.

@seanwash seanwash requested a review from drewclem May 14, 2021 22:39
@vercel
Copy link

vercel bot commented May 14, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/drewclem/protege/CKaDNATNGsFyEMEvdq1uZQ9CtX7K
✅ Preview: https://protege-git-feature-cleanup-eslint-errors-drewclem.vercel.app

@@ -1,4 +1,5 @@
import React from 'react'
// TODO: Formik is being imported but it not currently a dependency in package.json.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@drewclem This seems like it was left over from pre-next? As far as I can tell, this component isn't used at all. Should we just delete it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, safe to remove altogether

const [activeApplications, setActiveApplications] = useState()

useEffect(() => {
// TODO: What should happen with this fn? It's not being used anywhere.
// eslint-disable-next-line no-unused-vars
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

This component can be removed. It was original the dashboard view for company accounts, but that has been refactored

@@ -56,6 +56,7 @@ const JobList = () => {
<ul>
{activeApplications &&
activeApplications.map((job) => {
// TODO: ?
return <JobItem job={job} key={job.id} />
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@drewclem This component doesn't exist as far as I can tell. Should we remove any components like this from the jobs page for now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, that sounds good. I'm not sure how some of these came to be- other than getting left behind while iterating

@seanwash seanwash self-assigned this May 14, 2021
Copy link
Collaborator

@drewclem drewclem left a comment

Choose a reason for hiding this comment

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

This is looking great so far @seanwash

I see you have it marked as draft, should it be merged yet?

Also, I see you changed a lot of the p tags used for errors to spans. What's the significance of that?

const [activeApplications, setActiveApplications] = useState()

useEffect(() => {
// TODO: What should happen with this fn? It's not being used anywhere.
// eslint-disable-next-line no-unused-vars
Copy link
Collaborator

Choose a reason for hiding this comment

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

This component can be removed. It was original the dashboard view for company accounts, but that has been refactored

@@ -56,6 +56,7 @@ const JobList = () => {
<ul>
{activeApplications &&
activeApplications.map((job) => {
// TODO: ?
return <JobItem job={job} key={job.id} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, that sounds good. I'm not sure how some of these came to be- other than getting left behind while iterating

@sethburtonhall
Copy link
Contributor

This is looking good. Where are we on this PR? Can we merge it in?

@drewclem
Copy link
Collaborator

@sethburtonhall it's still in progress. @seanwash will flip it to Ready to Review when he's done with it

@seanwash
Copy link
Collaborator Author

Yep sorry, I'll try to finish this up this evening!

@sethburtonhall
Copy link
Contributor

Sounds good!!

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.

3 participants