Skip to content

Create GitHub JS lint action #136

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.

Sign up for GitHub

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

Jump to bottom
Merged
Sidsector9 merged 3 commits into from
Apr 16, 2023
Merged

Create GitHub JS lint action #136

Sidsector9 merged 3 commits into from
Apr 16, 2023

Conversation

@kmgalanakis
Copy link
Contributor

@kmgalanakis kmgalanakis commented Mar 23, 2023 •
edited
Loading

Description of the Change

  • Add a GitHub action to run the lining of the JS files
  • Add all the required ESLint-related files
  • Perform the necessary modifications to the assets/js/src/simple-page-ordering.js (the only active JS project file) to pass the linting tests.

Closes #130

How to test the Change

  • While running npm run dev, make changes to the assets/js/src/simple-page-ordering.js that would generate JS lint errors and verify that the errors are showing in the terminal.
  • Make changes to the assets/js/src/simple-page-ordering.js that would generate JS lint errors and run npm run js-lint and verify that the errors are showing in the terminal.
  • Push changes in a PR with some JS lint errors and verify that the JS linting job runs with a failure and produces the proper results (JS lint errors are supposed to be presented in the Files changed tab)
  • Push changes in a PR without any JS lint errors and verify that the JS linting job runs successfully

Changelog Entry

  • Added - GitHub action to run JS Lint tests.

Credits

Props @Sidsector9, @kmgalanakis

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@kmgalanakis kmgalanakis force-pushed the feature/gh-js-lint-action branch 6 times, most recently from 3c017e4 to 2619ccf Compare March 23, 2023 12:24
@kmgalanakis kmgalanakis force-pushed the feature/gh-js-lint-action branch from 2619ccf to 3007c58 Compare March 23, 2023 12:40
@kmgalanakis kmgalanakis self-assigned this Mar 23, 2023
@kmgalanakis kmgalanakis marked this pull request as ready for review March 23, 2023 13:03
@kmgalanakis kmgalanakis requested review from a team and jeffpaul as code owners March 23, 2023 13:03
@jeffpaul jeffpaul added this to the 2.5.0 milestone Mar 23, 2023
@jeffpaul jeffpaul requested review from a team and peterwilsoncc and removed request for a team and jeffpaul March 23, 2023 13:38
peterwilsoncc
peterwilsoncc reviewed Mar 26, 2023
View reviewed changes
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

Manually testing, there don't seem to be any side affects.

Just one question inline about the switch from importing jQuery.

assets/js/src/simple-page-ordering.js

// eslint-disable-next-line import/no-unresolved
import 'jquery-ui-sortable';
import 'jquery';
Copy link
Contributor

@peterwilsoncc peterwilsoncc Mar 26, 2023

Choose a reason for hiding this comment

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

As the plugin uses @wordpress/dependency-extraction-webpack-plugin, import jquery should work fine and allow us to avoid the need for window.jQuery throughout.

Were you experiencing issues with the import?

Copy link
Member

@jeffpaul jeffpaul Apr 10, 2023

Choose a reason for hiding this comment

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

@kmgalanakis could you please respond to Peter's comment above?

Copy link
Member

@jeffpaul jeffpaul Apr 14, 2023

Choose a reason for hiding this comment

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

@Sidsector9 want to handle any final updates here so this can get merged in as part of the 2.5.0 release?

Copy link
Contributor Author

@kmgalanakis kmgalanakis Apr 15, 2023

Choose a reason for hiding this comment

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

Sorry for the late response @peterwilsoncc and @jeffpaul. I had very limited bandwidth for the past couple of weeks.

To address @peterwilsoncc concern, I modified the eslint configuration to be able to use jQuery without having to do it through window.

This should be good to go @jeffpaul.

@jeffpaul jeffpaul mentioned this pull request Apr 14, 2023
Closed
16 tasks
Sidsector9
Sidsector9 approved these changes Apr 16, 2023
View reviewed changes
Copy link
Contributor

@Sidsector9 Sidsector9 left a comment

Choose a reason for hiding this comment

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

Thank you so much for the changes! LGTM 👍

@Sidsector9 Sidsector9 merged commit 90229c0 into develop Apr 16, 2023
@Sidsector9 Sidsector9 deleted the feature/gh-js-lint-action branch April 16, 2023 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@peterwilsoncc peterwilsoncc peterwilsoncc left review comments

@jeffpaul jeffpaul jeffpaul left review comments

@Sidsector9 Sidsector9 Sidsector9 approved these changes

Assignees

@kmgalanakis kmgalanakis

Labels

None yet

Projects

None yet

Milestone

2.5.0

Development

Successfully merging this pull request may close these issues.

Add GH JS linting action

4 participants

@kmgalanakis @peterwilsoncc @jeffpaul @Sidsector9