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

Closes #4357 on automate generating WPR css and js file #5239

Conversation

jeawhanlee
Copy link
Contributor

@jeawhanlee jeawhanlee commented Jul 12, 2022

Description

This PR tries to close the initiative of automating the generation of WPR CSS and js files.

Fixes #4357

Type of change

  • Enhancement (non-breaking change which improves an existing functionality)

Is the solution different from the one proposed during the grooming?

Slightly different from the proposed.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas

@jeawhanlee jeawhanlee self-assigned this Jul 12, 2022
@jeawhanlee jeawhanlee marked this pull request as ready for review July 12, 2022 20:51
@jeawhanlee jeawhanlee requested a review from a team July 12, 2022 20:51
Copy link
Contributor

@Tabrisrp Tabrisrp left a comment

Choose a reason for hiding this comment

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

That's a good start, but to avoid the issue with merge conflicts and still be able to review/test changes before deployement, we should modify the gulp tasks (or create new one), to generate unminified versions of the wpr-admin.css, wpr-admin-rtl.css and wpr-admin.js

@jeawhanlee jeawhanlee requested a review from Tabrisrp July 18, 2022 17:12
Copy link
Contributor

@Tabrisrp Tabrisrp 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 things:

  • Can you run the tasks so that the unminified build are sync'ed to git
  • Shouldn't the deploy build also use the .min suffix?
  • Could you update the enqueue code to be compatible with both no suffix and suffix, depending on the value of SCRIPT_DEBUG

@jeawhanlee jeawhanlee requested a review from Tabrisrp July 19, 2022 16:36
Copy link
Contributor

@Tabrisrp Tabrisrp left a comment

Choose a reason for hiding this comment

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

The minified files are added to gitignore but still commited, you can use git rm --cached <file> to remove them

inc/admin/ui/enqueue.php Outdated Show resolved Hide resolved
@jeawhanlee jeawhanlee requested a review from Tabrisrp July 21, 2022 11:10
Copy link
Contributor

@Tabrisrp Tabrisrp left a comment

Choose a reason for hiding this comment

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

only the map file remaining to be removed

@jeawhanlee jeawhanlee requested a review from Tabrisrp July 21, 2022 18:07
@engahmeds3ed
Copy link
Contributor

Do we need QA for this one?

@engahmeds3ed
Copy link
Contributor

@piotrbak
@wp-media/qa
do we need QA for this one?

@piotrbak
Copy link
Contributor

piotrbak commented Aug 2, 2022

@engahmeds3ed How we could test this PR?

@engahmeds3ed
Copy link
Contributor

@engahmeds3ed How we could test this PR?

I don't think we can test that!

@jeawhanlee @Tabrisrp what do u think?

@jeawhanlee
Copy link
Contributor Author

@engahmeds3ed How we could test this PR?

I don't think we can test that!

@jeawhanlee @Tabrisrp what do u think?

I don't think QA will be able to test this, we might have to run a fake deploy to test this, though I tested this using the on:push and pull rquest git action, so I think @Tabrisrp would know a better way to test this.

@Tabrisrp
Copy link
Contributor

Tabrisrp commented Aug 2, 2022

Yeah we don't need QA, if it was tested using different triggers, it should be fine on the release tag too.

@Tabrisrp Tabrisrp merged commit 709f571 into develop Aug 3, 2022
@Tabrisrp Tabrisrp deleted the enhancement/4357-automate-asset-files-generation-before-deployment branch August 3, 2022 19:10
@mostafa-hisham
Copy link
Contributor

@jeawhanlee @Tabrisrp
do we need to update the GitHub action to run the gulp task to generate unminified files too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

automate generating wp-rocket CSS and JS files after deployment
5 participants