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

Replace custom build process with WP-Scripts #282

Merged
merged 15 commits into from Oct 12, 2023
Merged

Conversation

iamdharmesh
Copy link
Member

Description of the Change

PR replaces current custom build process with WP-Scripts also removes some legacy files which no longer needed

Closes #193

How to test the Change

  1. Run npm install
  2. Run npm run build
  3. Make sure build files are generated successfully.
  4. Smoke test plugin and make sure everything working fine

Changelog Entry

Changed - Replace custom build process with WP-Scripts

Credits

Props @iamdharmesh @peterwilsoncc

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.

@iamdharmesh iamdharmesh requested review from a team and ravinderk and removed request for jeffpaul and a team September 8, 2023 12:23
@github-actions github-actions bot added this to the 2.2.0 milestone Sep 8, 2023
@github-actions github-actions bot added the needs:code-review This requires code review. label Sep 8, 2023
ravinderk
ravinderk previously approved these changes Sep 8, 2023
Copy link
Contributor

@ravinderk ravinderk left a comment

Choose a reason for hiding this comment

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

@iamdharmesh The Pull request is looking good to me. I left a minor suggestion and a new issue which is not a blocker.

Let me know if you have any questions.

Additional issues:

  • The Twitter icon is middle aligned in the column, which looks odd with other
image

includes/admin/assets.php Show resolved Hide resolved
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.

A couple of notes inline.

package.json Outdated Show resolved Hide resolved
webpack.config.js Outdated Show resolved Hide resolved
@jeffpaul
Copy link
Member

jeffpaul commented Oct 9, 2023

@ravinderk @peterwilsoncc looking for updated review, thanks!

Copy link
Contributor

@ravinderk ravinderk left a comment

Choose a reason for hiding this comment

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

@iamdharmesh

I noticed the following:

  • Eslint list issues in the pull request.
  • E2E test GitHub workflow is failing.
  • Few missing text domains from translatable strings.
    image
    image

Let me know if you have questions.

@iamdharmesh
Copy link
Member Author

iamdharmesh commented Oct 12, 2023

@ravinderk I have addressed PR feedback. Also, E2E tests are failing as we use build-release-zip.yml from develop and have made some changes in the build scripts. I temporarily pointed it to this PR branch to ensure E2E tests are running fine in this commit and it's E2E tests are passing. currently, I have rolled back it.

@ravinderk ravinderk merged commit 34564ff into develop Oct 12, 2023
13 of 14 checks passed
@ravinderk ravinderk deleted the enhancement/193 branch October 12, 2023 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-review This requires code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace custom build process with WP-Scripts.
5 participants