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

Lint JS and CSS #85

Merged
merged 22 commits into from
Feb 16, 2024
Merged

Lint JS and CSS #85

merged 22 commits into from
Feb 16, 2024

Conversation

psrpinto
Copy link
Member

@psrpinto psrpinto commented Feb 15, 2024

This is the final linting PR. It's mostly just fixing whitespace. In the js, I refactored a little to extract functions out of functions.

This will likely cause conflicts with #74. I can help resolve conflicts there, or it if gets merged, I will resolve conflicts here.

Diff ignoring whitespace changes

@psrpinto psrpinto changed the base branch from trunk to linting-3 February 15, 2024 16:10
@psrpinto psrpinto changed the title Lint js Lint JS and CSS Feb 15, 2024
Base automatically changed from linting-3 to trunk February 15, 2024 17:59
@psrpinto psrpinto marked this pull request as ready for review February 16, 2024 11:48
Copy link
Member

@akirk akirk left a comment

Choose a reason for hiding this comment

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

Looks good! I am just confused regarding the tooling, shall we add wp-scripts for this, like in https://github.com/GlotPress/gp-translation-helpers/blob/main/composer.json ?

@psrpinto
Copy link
Member Author

Looks good! I am just confused regarding the tooling, shall we add wp-scripts for this, like in https://github.com/GlotPress/gp-translation-helpers/blob/main/composer.json ?

(I'm assuming you meant package.json instead of composer.json)

It didn't occur to me to use wp-scripts, good point. I'm willing to make the change to use wp-scripts instead of phpcs, if you think it makes sense.

Currently we don't yet have any npm/node dependencies, so we could consider sticking with phpcs for now. (Would save 384MB in the node_modules/ 😅 ).

@psrpinto
Copy link
Member Author

I rebased with trunk to fix conflicts.

@akirk
Copy link
Member

akirk commented Feb 16, 2024

(I'm assuming you meant package.json instead of composer.json)

Oops, indeed.

Currently we don't yet have any npm/node dependencies, so we could consider sticking with phpcs for now.

I wasn't aware that phpcs can do this reformatting. I guess it's ok for the moment :-)

@psrpinto
Copy link
Member Author

Ok, cool. I think it's a good time to merge this one now because there's only #74 which will be affected. Merging.

@psrpinto psrpinto merged commit 563ccef into trunk Feb 16, 2024
2 checks passed
@psrpinto psrpinto deleted the lint-js branch February 16, 2024 15:53
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.

2 participants