Skip to content

Conversation

@GordonSmith
Copy link
Member

@GordonSmith GordonSmith commented Oct 23, 2025

Checklist:

  • The commit message is properly formatted and free of typos.
    • The commit message title makes sense in a changelog, by itself.
    • The commit message includes a "fixes" reference if appropriate.
    • The commit is signed.
  • The change has been fully tested:
    • I have viewed all related gallery items
    • I have viewed all related dermatology items
  • There are no similar instances of the same problem that should be addressed
    • I have addressed them here
    • I have raised new issues to address them separately

Testing:

@GordonSmith GordonSmith requested a review from Copilot October 23, 2025 12:55
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds keyboard navigation functionality to Column and Pie chart components, enabling users to navigate and interact with chart elements using the Tab key and Space/Enter keys for selection. The changes also update repository references from "trunk" to "main" branch across multiple package.json files.

Key changes:

  • Adds keyboard navigation support with Tab, Space, and Enter keys for Column and Pie charts
  • Implements ARIA attributes and focus states for improved accessibility
  • Updates CSS styling for hover, focus, and selection states with visual feedback
  • Updates homepage URLs from "trunk" to "main" branch in package.json files

Reviewed Changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
packages/util/package.json Updated homepage URL from trunk to main branch
packages/observablehq-compiler/package.json Updated homepage URL from trunk to main branch
packages/markdown-it-plugins/package.json Updated homepage URL from trunk to main branch (with typo)
packages/dataflow/package.json Updated homepage URL from trunk to main branch
packages/chart/src/Pie.ts Added keyboard navigation, ARIA attributes, and slice padding functionality to Pie chart
packages/chart/src/Pie.css Added CSS for focus states, hover effects, and keyboard navigation styling
packages/chart/src/Column.ts Added keyboard navigation, ARIA attributes, and Tabster integration to Column chart
packages/chart/src/Column.css Updated CSS for selected state, added focus and hover styles
packages/chart/index.html Added demo controls for testing keyboard navigation on both chart types

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@GordonSmith GordonSmith force-pushed the main branch 3 times, most recently from ac285a3 to 136469b Compare October 29, 2025 15:37
@GordonSmith GordonSmith requested a review from Copilot November 26, 2025 14:00
@GordonSmith GordonSmith marked this pull request as ready for review November 26, 2025 14:05
Copilot finished reviewing on behalf of GordonSmith November 26, 2025 14:05
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 17 out of 18 changed files in this pull request and generated 24 comments.

Comments suppressed due to low confidence (2)

packages/common/src/TitleBar.ts:36

  • The button element (anchor tag) should have role="button" and tabindex="0" attributes for proper accessibility. While anchor tags are naturally focusable, explicitly setting these attributes ensures correct ARIA semantics when the element behaves as a button rather than a link.
        this._i = this._element
            .attr("href", "#")
            .on("click", function () {
                context.click();
                d3Event.preventDefault();
            })
            .on("keydown", function (this: HTMLElement) {
                if (d3Event.key === " " || d3Event.key === "Spacebar" || d3Event.code === "Space") {
                    this.click();
                    d3Event.preventDefault();
                }
            })
            .on("mousemove", this.mouseMove)
            .on("mouseout", this.mouseOut)
            .append("i")
            ;

packages/graph/tests/test6.ts:75

  • Unused variable g.
        const g = genData2();

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

Signed-off-by: Gordon Smith <GordonJSmith@gmail.com>
Copy link
Contributor

@jeclrsg jeclrsg left a comment

Choose a reason for hiding this comment

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

@GordonSmith looked good. Just one nitpick about some comments and console.logs that got left behind?

Was taking a while to get through, until I remembered to tick the "hide whitespace" checkbox. I wish you could get GH to save that as a preference or something

window.__scatter?.resize()?.render();
}

// Add event listener for the checkbox
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick, a few unnecessary comments and console.logs here

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants