Skip to content

feat(chart): add ChoroplethMap#610

Open
AnmolSaini16 wants to merge 5 commits into
cloudflare:mainfrom
AnmolSaini16:feat/choropleth-map
Open

feat(chart): add ChoroplethMap#610
AnmolSaini16 wants to merge 5 commits into
cloudflare:mainfrom
AnmolSaini16:feat/choropleth-map

Conversation

@AnmolSaini16

@AnmolSaini16 AnmolSaini16 commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Adds ChoroplethMap, a GeoJSON region choropleth chart component with Kumo light/dark map colors, tooltip formatting, optional legend, hover/click callbacks, roam controls, docs, demos, exports, and a changeset.

Reviews

  • bonk has reviewed the change
  • automated review not possible because: GitHub review automation is not available from this environment

Tests

  • Tests included/updated
  • Automated tests not possible - manual testing has been completed as follows: ran pnpm --filter @cloudflare/kumo-docs-astro typecheck with 0 errors; existing warnings/hints remain unrelated
  • Additional testing not necessary because:

@stritt

stritt commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

/bonk please review

@pkg-pr-new

pkg-pr-new Bot commented Jun 30, 2026

Copy link
Copy Markdown
npm i https://pkg.pr.new/@cloudflare/kumo@610

commit: cadeb3c

@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Docs Preview

View docs preview

Commit: cadeb3c

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR! The ChoroplethMap implementation is well-structured — clean viewport preservation, consistent event handling with BubbleMap, and nice Kumo color integration.

A few things to address before merging:

  1. BubbleMap default change is undocumented. Switching the default from → changes the default interaction behavior for existing consumers. If this is intentional it should be called out in the changeset.
  2. Missing ChoroplethMap PropsTable in docs. The API Reference section should include a PropsTable for the new component too.
  3. Test page consistency. The ChoroplethMap test section is missing a descriptive paragraph like the other sections have.

See inline comments for details.

Comment thread packages/kumo/src/components/chart/Maps.tsx
Comment thread packages/kumo-docs-astro/src/pages/charts/maps.astro
Comment thread packages/kumo-docs-astro/src/pages/tests/map.astro Outdated
@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor
Visual Regression Report — 25 changed, 16 unchanged

25 screenshot(s) with visual changes:

Button / Basic

408 px (0.4%) changed

Before After Diff
Before After Diff

Button / Variant: Primary

698 px (0.69%) changed

Before After Diff
Before After Diff

Button / Variant: Outline

268 px (0.26%) changed

Before After Diff
Before After Diff

Button / Sizes

1,507 px (1.48%) changed

Before After Diff
Before After Diff

Button / Loading State

21 px (0.02%) changed

Before After Diff
Before After Diff

Button / Title

660 px (0.65%) changed

Before After Diff
Before After Diff

Dialog / Dialog With Actions

353 px (0.35%) changed

Before After Diff
Before After Diff

Dialog / Dialog Sizes

3,057 px (3.01%) changed

Before After Diff
Before After Diff

Dialog / Dialog Alert

912 px (0.9%) changed

Before After Diff
Before After Diff

Dialog / Dialog Confirmation

853 px (0.84%) changed

Before After Diff
Before After Diff

Dialog / Custom Max Width

739 px (0.73%) changed

Before After Diff
Before After Diff

Dialog / Dialog With Select

383 px (0.38%) changed

Before After Diff
Before After Diff

Dialog / Dialog With Combobox

493 px (0.49%) changed

Before After Diff
Before After Diff

Dialog / Dialog With Dropdown

493 px (0.49%) changed

Before After Diff
Before After Diff

Select / Select Basic

859 px (0.85%) changed

Before After Diff
Before After Diff

Select / Select Sizes

2,860 px (1.54%) changed

Before After Diff
Before After Diff

Select / Select Without Label

746 px (0.73%) changed

Before After Diff
Before After Diff

Select / Select With Error

1,979 px (1.67%) changed

Before After Diff
Before After Diff

Select / Select With Tooltip

853 px (0.84%) changed

Before After Diff
Before After Diff

Select / Select Loading

2,360 px (1.17%) changed

Before After Diff
Before After Diff

Select / Select Multiple

1,934 px (1.91%) changed

Before After Diff
Before After Diff

Select / Select Disabled Options

1,476 px (1.45%) changed

Before After Diff
Before After Diff

Select / Select Disabled Items

735 px (0.72%) changed

Before After Diff
Before After Diff

Select / Select Grouped

1,022 px (1.01%) changed

Before After Diff
Before After Diff

Select (Open)

0 px (0%) changed

Before After Diff
Before After Diff
16 screenshot(s) unchanged
  • Button / Variant: Secondary
  • Button / Variant: Ghost
  • Button / Variant: Destructive
  • Button / Variant: Secondary Destructive
  • Button / With Icon
  • Button / Icon Only
  • Button / Disabled State
  • Button / Link as Button
  • Dialog / Dialog Basic
  • Dialog (Open)
  • Select / Select With Description
  • Select / Select Placeholder
  • Select / Select Custom Rendering
  • Select / Select Complex
  • Select / Select Grouped With Disabled
  • Select / Select Long List

Generated by Kumo Visual Regression

@ask-bonk

ask-bonk Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Follow-up: my earlier review comments lost some backticks due to escaping issues. Here is the same feedback with proper formatting:

  1. BubbleMap roam default change is undocumented
    In Maps.tsx, the BubbleMap roam default changed from true to false. The tame-maps-walk.md changeset only mentions the viewport-preservation fix, but existing consumers who omit the roam prop will lose drag-to-pan/scroll-to-zoom on upgrade. Please either revert the default to true (the appliedGeoRef fix already solves the reset issue) or update the changeset to document this breaking interaction change.

  2. Missing ChoroplethMap PropsTable in docs
    In maps.astro, the API Reference section should include a PropsTable for the new component too. Add this after the BubbleMap PropsTable:

    <Heading level={3} class="mb-2 mt-4 text-lg">ChoroplethMap</Heading>
    <PropsTable component="ChoroplethMap" />
  3. Test page consistency
    In map.astro, the ChoroplethMap test section is missing a descriptive paragraph like the other sections have. Consider adding something like:

    <p class="text-kumo-subtle">
      Choropleth map shading regions by value with continuous colour scaling.
    </p>

Otherwise the implementation looks solid. Nice work!

@ask-bonk

ask-bonk Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review submitted on PR #610.

Summary of feedback:

  1. BubbleMap roam default change is undocumented — the default switched from true to false but the changeset only mentions the viewport-preservation fix. Existing consumers who don’t set roam explicitly will lose pan/zoom on upgrade. The author should either revert the default or update the changeset.
  2. Missing PropsTable for ChoroplethMap in the docs page’s API Reference section.
  3. Test page consistency — the ChoroplethMap test section lacks a descriptive paragraph like the other sections.

The batch review was posted successfully (with inline comments on the relevant lines), and a follow-up top-level comment clarifies the properly formatted feedback.

github run

- Default both maps to a latitude-clamped Mercator projection (d3-geo);
  add `projection` prop (custom d3-geo projection or `null` for raw plotting)
- Size the chart container by the projected window's aspect ratio so maps
  fill the frame without letterboxing; `height` still overrides
- Make ChoroplethMap borderless (minimalist, matching BubbleMap); drop the
  now-unused MapColors.line seam colour
- Add a Cloudflare network hero example (major locations in brand orange with
  city + IATA tooltips) as the first thing on the Maps docs page
@rozenmd

rozenmd commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

/bonk can you review?

@ask-bonk

ask-bonk Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

@rozenmd Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

@rozenmd

rozenmd commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

/bonk can you try again

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR! The ChoroplethMap implementation is solid — clean viewport preservation, consistent event handling with BubbleMap, and nice Kumo color integration.

However, the three issues from the previous review are still unaddressed. Please take a look at the inline comments below.

Comment thread packages/kumo/src/components/chart/Maps.tsx
Comment thread packages/kumo-docs-astro/src/pages/charts/maps.astro
Comment thread packages/kumo-docs-astro/src/pages/tests/map.astro Outdated
@ask-bonk

ask-bonk Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review submitted on PR #610.

Summary of feedback:

All three issues from the prior review are still unaddressed:

  1. BubbleMap roam default change is undocumented — the default switched from true to false, which changes the default interaction behavior for existing consumers. The tame-maps-walk.md changeset only mentions viewport preservation. The author should either revert the default or update the changeset to call out this breaking change.
  2. Missing PropsTable for ChoroplethMap in the docs page's API Reference section.
  3. Test page consistency — the ChoroplethMap test section lacks a descriptive paragraph like the other sections.

The batch review was posted successfully with inline comments on the relevant lines, including suggested code changes where applicable.

github run

@stritt

stritt commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

/bonk can you review

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All three issues from the previous review rounds are now resolved:

  1. Changeset documents the breaking change — the BubbleMap roam default switch from truefalse is explicitly called out in .changeset/choropleth-map.md.
  2. Docs include ChoroplethMap PropsTablemaps.astro now has the API reference for both map components.
  3. Test page is consistent — all sections now follow the same structure.

The viewport-preservation logic (appliedGeoRef/appliedViewRef), d3-geo projection integration, and Kumo-themed colour ramps are all well implemented. The choropleth visualMap setup and tooltip escaping look correct.

Two non-blocking nits:

  • MapProjection is exported from Maps.tsx but not re-exported from the chart barrel (src/components/chart/index.ts) or the package root, so consumers typing custom projections need a deep import.
  • The switch to sqrt(v / vmax) for bubble radius (scaling from zero instead of vmin) is a subtle behaviour change worth keeping in mind for release notes, even though it's an improvement.

Nice work — LGTM!

@ask-bonk

ask-bonk Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Review submitted on PR #610.

Summary: All three blocking issues from the prior review rounds are resolved:

  1. Changeset documents the breaking change — the BubbleMap roam default switch from truefalse is explicitly called out.
  2. Docs include ChoroplethMap PropsTablemaps.astro now has API references for both map components.
  3. Test page is consistent — all sections now follow the same structure.

The implementation is solid: clean viewport preservation via appliedGeoRef/appliedViewRef, nice d3-geo projection integration, and well-tuned Kumo colour ramps for both light and dark modes. I approved the PR with two non-blocking nits about re-exporting the MapProjection type and the subtle bubble-radius formula change.

github run

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.

3 participants