Contributing a style change to the Learn WordPress codebase results in file conflicts.

[jonathanbossenger]

When fixing CSS issues in any of the wporg-learn-2020 theme scss files, the wporg-learn-2020/css/style.css and wporg-learn-2020/css/style-rtl.css are reported as having conflicts in the PR

Example PRs #1362, #1366, #1368, #1375, #1388.

Also seems to be the reason for conflicts on #995, #1160, #1348

From experience, this seems to be because those files are built every time the yarn build process runs.

Ideally, we should either a) have a way to create PRs that don't cause these conflicts, or b) have a guideline around whether it's acceptable to merge PRs with these specific conflicts.

@adamwoodnz is the build process run before any updated code is deployed to production?

If this is the case, then we can simply remove those two files from revision control, and add them to the .gitignore, meaning they won't be included in any future PRs.

[alexstine]

Maybe it makes since to build these files via GitHub Actions only on trunk commit?

[adamwoodnz]

Yeah correct, everytime someone merges a PR these files are rebuilt, and then any existing PRs with style changes will be in conflict.

It does make sense to remove them from source control. I'm wondering if they have been included up until now so that the local environment can be run without building.

In terms of when to build there are a few approaches we could take. We currently build when syncing this repo with SVN, and we could possibly add the built files then so that they are sync'd and deployed. I might experiment with this as it stands, as it seems the lowest effort fix.

Alternatively we could create and update a build branch when merging to trunk, and use that branch when syncing.

[renintw]

It does make sense to remove them from source control.

Agree not to include them in the src control.

In terms of when to build there are a few approaches we could take. We currently build when syncing this repo with SVN, and we could possibly add the built files then so that they are sync'd and deployed. I might experiment with this as it stands, as it seems the lowest effort fix. Alternatively we could create and update a build branch when merging to trunk, and use that branch when syncing.

Building the repo and syncing files to SVN through the sync script is probably the easiest way to fix the issue. We just need to add those built files into .gitignore.

[adamwoodnz]

We'd need to ensure a clear instruction in the readme that the style needs to be built when setting up, or it could lead to issues being raised.

[renintw]

I saw there's yarn run create in the setup section, was thinking that is enough for starting the local env successfully. Or I could have missed some other context here.

[jonathanbossenger]

Thanks for the insight here, folks.

I saw there's yarn run create in the setup section, was thinking that is enough for starting the local env successfully. Or I could have missed some other context here.

I agree this is enough for most folks, and if not, we can always update the readme to be more descriptive about what this does, and why it's required to run the site locally.

I think we're in agreement that removing these files (and any other files that might be built in this way) from source control, and then building the repo and syncing the files to SVN through the sync script is a solid plan, bonus points for it being the lowest effort.

@adamwoodnz can I leave this one with you to take care of?

[adamwoodnz]

@adamwoodnz can I leave this one with you to take care of?

Yes, although we have other high priority work on currently so I'm unsure how soon I can get to it.

[jonathanbossenger]

No worries. In the meantime, I'll ask folks in other PRs to check out the original versions of those files.

[renintw]

@adamwoodnz I took a look at the Gruntfile when working on a small simple task and it seems that only style.css, print.css, and style-rtl.css would be generated in the CSS folder. So I added them to the .gitignore file in this PR I just opened for the task.

[adamwoodnz]

Given this outcome, can we now close this @renintw ?

Thanks for handling this 🌟

[renintw]

@adamwoodnz Yeah, it's good to close 🎉

Closed as fixed in #1448.