Skip to content

[Feat] Introduce linting in frontend project #187

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

Closed
wants to merge 29 commits into from

Conversation

stefanoamorelli
Copy link
Contributor

  • chore: add eslint configuration
  • chore: add lintrc file
  • refactor: fix typecheck issues
  • fix(lint): linting errors
  • ci: add frontend lint check

Solves: #133


We introduce ESlint for the frontend/, fix the existing issues, and add a Github workflow to run the linting and prevent merging of code that does not pass the linting.

@stefanoamorelli
Copy link
Contributor Author

@microsoft-github-policy-service agree

@stefanoamorelli
Copy link
Contributor Author

@matheusmaldaner as promised, here you are! :)

Would appreciate your through review as the PR is medium-sized due to fixing the existing linting error.

@matheusmaldaner
Copy link
Collaborator

Hi @stefanoamorelli, thank you so much for all the changes! Could you review my minor commit? Your linting implementation highlighted some other cases that were failing with the yarn typecheck command, which I have addressed.

Do you know why the Frontend Checks (frontend-lint) are failing in the GitHub CI workflow? I ran the code on my end and there are no errors.

Copy link
Collaborator

@matheusmaldaner matheusmaldaner left a comment

Choose a reason for hiding this comment

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

Good linting changes, do not seem to change anything that could break the code. All tests (poe check) are still passing.

@stefanoamorelli
Copy link
Contributor Author

@matheusmaldaner good stuff! I've reviewed your commit, LGTM left just a couple of comments to confirm.

Also, I've pushed a commit to allow warnings to allow the CI/CD to pass with warnings (given that we don't introduce more warnings).

@matheusmaldaner
Copy link
Collaborator

Requested a review from @husseinmozannar on this so we can merge it before too many changes are made to the system and also so that he can maybe shed light into why the first check is still failing

@stefanoamorelli
Copy link
Contributor Author

Requested a review from @husseinmozannar on this so we can merge it before too many changes are made to the system and also so that he can maybe shed light into why the first check is still failing

Sounds good 👌

matheusmaldaner and others added 2 commits June 26, 2025 21:42
Co-authored-by: matheusmaldaner <t-mkunzler@microsoft.com>
microsoft#185)

Co-authored-by: 侯瑞 <hourui@xiaohongshu.com>
Co-authored-by: Hussein Mozannar <hssein.mzannar@gmail.com>
@stefanoamorelli
Copy link
Contributor Author

@matheusmaldaner I've rebased the branch and pushed another commit to try fix the failing linting on CI.

Is there anything I can do to help unblock this? Happy to help!

@matheusmaldaner
Copy link
Collaborator

Hi @stefanoamorelli the first check still fails and I am unsure why :/

Could you look into this whenever you got the time or let me know if there is anything I can do on my end to help us solve this issue? Thank you a lot for the contributions and the help with this

husseinmozannar and others added 4 commits June 29, 2025 23:37
Co-authored-by: Tyler Payne <tylerpayne@microsoft.com>
Co-authored-by: Hussein Mozannar <hssein.mzannar@gmail.com>
Co-authored-by: Hussein Mozannar <hmozannar@microsoft.com>
vrashankshetty and others added 16 commits June 29, 2025 23:37
Co-authored-by: Hussein Mozannar <hmozannar@microsoft.com>
Co-authored-by: Hussein Mozannar <hssein.mzannar@gmail.com>
Co-authored-by: Tyler Payne <tylerpayne@microsoft.com>
Co-authored-by: Hussein Mozannar <hmozannar@microsoft.com>
Co-authored-by: Tyler Payne <tylerpayne@microsoft.com>
Co-authored-by: Tyler Payne <tylerpayne@microsoft.com>
Co-authored-by: Hussein Mozannar <hssein.mzannar@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: matheusmaldaner <t-mkunzler@microsoft.com>
Fix playwright fill id to use coordinates rather than element typing.

Fix already covered by playwright tests

Better simulates how real users would interact with the website by sending actual keyboard events to the browser
Co-authored-by: Hussein Mozannar <hssein.mzannar@gmail.com>
Co-authored-by: Hussein Mozannar <hmozannar@microsoft.com>
@stefanoamorelli stefanoamorelli marked this pull request as draft June 29, 2025 20:42
@stefanoamorelli
Copy link
Contributor Author

stefanoamorelli commented Jun 29, 2025

@matheusmaldaner thank you! The branch was outdated and I must have corrupted the git history when resolving all the conflicts (there were quite a few as the linting fixes touched a lot of files).

I've created a new PR with the most up to date branch and the linting should pass there (it was failing because new errors appeared when rebasing): #234

I'm closing this one and we can continue the conversation in the new PR 🙏

(I've also split the changes in atomic commits as an effort to make review easier for you!)

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.

7 participants