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

Clean up several minor issues in setPage #701

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

aarongoldenthal
Copy link
Contributor

The following issues were noticed today in the setPage function (all are low priority cleanup).

  • Removed unused url argument from setPage
  • Moved debug log entry for navigating to URL to proper location (from setPage to gotoUrl)
  • Update debug logging for all setPage operations

Debug log changes:

 > pa11y -d https://pa11y.org

 Welcome to Pa11y

  > Running Pa11y on URL https://pa11y.org
  > Debug: Launching Headless Chrome
- > Debug: Opening URL in Headless Chrome
+ > Debug: Opening new browser page
+ > Debug: Setting page User-Agent to "pa11y/8.0.0"
+ > Debug: Setting page viewport
+ > Debug: Navigating to https://pa11y.org
  > Debug: Loading runner: htmlcs
  > Debug: Injecting Pa11y
  > Debug: Injecting runner: htmlcs
  > Debug: Running Pa11y on the page
  > Debug: Document title: "Pa11y"

 No issues found!

@danyalaytekin danyalaytekin self-requested a review April 8, 2024 05:24
Copy link
Member

@danyalaytekin danyalaytekin left a comment

Choose a reason for hiding this comment

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

(Oops, let me try posting this again using the review button.)

Well spotted @aarongoldenthal!

  1. I think you've completed a reversion which hadn't quite concluded. But this happened a while ago so I should check... are you happy with this signature change @josebolos or was it left there pending a restoration of the reverted feature?
  2. Checking that commit I also noticed that we have some testing around the debug logging, which it could make sense to expand to cover the changes in this PR, for consistency, if that's our practice. Alternatively the extra logging could be pushed into a separate PR. @aarongoldenthal @josebolos

@danyalaytekin danyalaytekin added this to the 8.x milestone Apr 8, 2024
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.

None yet

2 participants