StatTag / StatWrap

StatWrap
https://sites.northwestern.edu/statwrap/
MIT License
1 stars 10 forks source link

[FIX] : Resolves ESLint Failures πŸ› #197

Closed AdiAkhileshSingh15 closed 6 months ago

AdiAkhileshSingh15 commented 6 months ago

Changes Overview

This PR aims to address unwanted ESlint failures and false positive errors by implementing the following changes:

Related Issue πŸ›

Fixes #169

Checklist βœ…

Following each commit, the following checks were executed locally to ensure the functionality remains intact:

Commands used:

Additional checks:

Reviewer

@lrasmus

AdiAkhileshSingh15 commented 6 months ago

@lrasmus, I wanted to highlight the 3rd commit in this PR (acf3c3d), which focuses on resolving eslint issues using lint-fix. Since it involves numerous files and only addresses linting errors without introducing any breaking changes, I've already reviewed and confirmed each syntax correction. Hence, I recommend skipping a detailed review of this commit as it primarily consists of eslint syntax fixes.

AdiAkhileshSingh15 commented 6 months ago

@lrasmus do check the introduction of title dependency in app/components/EditableSelect/EditableSelect.js useEffect.

lrasmus commented 6 months ago

@lrasmus do check the introduction of title dependency in app/components/EditableSelect/EditableSelect.js useEffect.

Thanks for mentioning that! I did notice it, and when testing it didn't seem to impact the functionality (from what I could tell). Curious if you know why the linter recommended it, and how strong the recommendation is? I'm okay keeping it if there is a good reason. I'm equally okay ignoring the linter's suggestion there and reverting to how it was before.

AdiAkhileshSingh15 commented 6 months ago

@lrasmus Actually, linter suggests adding all dependencies in the useEffect dependency array to ensure it triggers appropriately. While there are similar suggestions across our project, this one appeared reasonable to implement compared to others. I've opted not to disable this warning globally, as some suggestions may prove helpful in the future. We can selectively disable them based on how strong the recommendation is.

lrasmus commented 6 months ago

I think that sounds like a good approach @AdiAkhileshSingh15!

AdiAkhileshSingh15 commented 6 months ago

@lrasmus I've updated the branch with latest changes in master. It's ready to merge, if all reviews are done.

AdiAkhileshSingh15 commented 6 months ago

@lrasmus please check latest push

lrasmus commented 6 months ago

Amazing, thank you so much @AdiAkhileshSingh15 !