act-rules / act-rules.github.io

Accessibility conformance testing rules for HTML
https://act-rules.github.io/
Other
131 stars 67 forks source link

Do branch publish for easier reviewing of PRs #2183

Open Jym77 opened 2 months ago

Jym77 commented 2 months ago

Some rules can have complex examples, especially when shared assets are used. This makes is quite hard to review them without being able to render the examples. See https://github.com/act-rules/act-rules.github.io/pull/2167#issuecomment-2047786194

We could re-use the Github pages setup to do a "branch publish" of every PR, we can point to it from the PR and thus make it easier to see the result of the code.

Jym77 commented 2 months ago

šŸ¤” OK, doing a bit of digging through our (broken) Github pages deploy system.

It seems that ā€¢ dispatch.yml is sending event to act-rules-web on every push on develop. ā€¢ This is taken by build.yml on the other side, which does some magic with the markdown ā€¢ Build.yml is then supposed to send back an event to the initial repo, this is apparently broken (although the curl comment does get an answer, so the workflow doesnā€™t fail). But some still seem to work šŸ¤” ā€¢ This is taken on the original side by publish.yml, which is just supposed to copy everything in the master branch, used for Github pages deploy. ā€¢ Publish.yml seems broken for lack of lockfile. No recent action has succeeded. That probably explains our lack of publications on Github pages recently.

Iā€™m not sure how well we can integrate that with deploy-pr-previewā€¦

=> It should be possible to setup a /prs/ directory (or similar) in the master branch of the main repo; then update dispatch.yml to react on any PR (not just push to develop); then on the pushback to publish.yml, we could have a payload indicating the original branch, and copy to prs/XXX instead of ā€œ.ā€ To let Github pages do their magic (plus maybe message on the PR itself). This is manually doing what deploy-pr-preview is doing. => Another possibility to leverage deploy-pr-preview would be for publish.yml to create a new branch and PR with a special name, and then let deploy-pr-preview work on these PRs. That sounds needlessly complicated and doesnā€™t save us the hard problems of branch management and co, plus double the number of opened PRs šŸ™ˆ

Given the mess that is the master branch (something to do with how Github pages are built, I guess), I donā€™t think it would be a good idea for each branch to contain that kind of data. I also assume the current setup was build to keep the markdown files ā€œaloneā€ in their repo and make it easier to understand (and avoid polluting PRs with unrelated diffsā€¦) So Iā€™m reluctant to do deeper changes to setupā€¦

Since Github pages seems to de quite a lot of magic on its own, we only have to solve ā€œsimpleā€ problems of (a) passing the correct payload back and forth (dispatch events are made for that); (b) copying to the correct directory instead of ā€œ.ā€; and (c) putting back a comment on the initial PR (we can probably steal from deploy-pr-preview for that, or just ignore it to start with and let the PR author or somebody else manually copy itā€¦) Oh, and also figuring out what is making our current publish.yml unhappy and breaking the deployment.

Or maybe we could transfer ownership of the repo to the W3C org and use Netlify šŸ¤” Iā€™m not sure why the ACT Rules org is separated from the W3C oneā€¦ (except for allowing Github pagesā€¦ which we donā€™t really need now that the main deploy is on the WAI siteā€¦)

dd8 commented 1 week ago

I've put together a Hugo based preview generator for the rules pages, which you can run locally to build a preview of any branch. It only needs Hugo installed (which is a single binary with no dependences) plus the files in this repo. Hugo does all the heavy lifting (e.g. syntax highlighting) so the config and templates are very minimal (41 lines of code in total, excluding the CSS).

https://github.com/powermapper/act-rules-preview

This is what the generated output looks like:

Screenshot 2024-06-24 at 07 12 00