search

pa11y / pa11y-ci

Pa11y CI is a CI-centric accessibility test runner, built using Pa11y
https://pa11y.org
GNU Lesser General Public License v3.0
519 stars 63 forks source link

feat: add result title to results file default to url #99

Closed stbth01 closed 10 months ago

stbth01 commented 4 years ago

I have never put an open source PR in before so I feel like I'm doing stuff wrong please let me know I'm hoping to learn.

This PR is related to my issue here (I don't know how to link them sorry): https://github.com/pa11y/pa11y-ci/issues/98

danyalaytekin commented 10 months ago

Hi @stbth01, thanks for this contribution and sorry for not responding to it sooner. I hope that didn't put you off and that you've gone on to make many more open-source contributions. You did indeed succeed in linking this PR to that issue and your implementation will be visible to anyone reading there and picking up the thread, so I'm going to close this PR in favour of that issue, since we're pushing for pa11y-ci@4 and this won't be revisited before then. This PR can be reopened if everyone decides to go this way, or we could cherry-pick the commit you've authored into a different PR.

My other thoughts:

  1. I see people were quite interested in this issue, and there's been much more discussion of it since it was raised and this PR created. The suggestion of a description looks similar to your use of title, but I haven't read it all in depth yet.
  2. When functionality is added, we have to update the tests to cover it. We're slightly beyond the 90 days of GitHub log retention now so I can't check out whether they did still pass, but it looks like we'll need to add a bit more coverage as well to cover the new logic, to be safe.