feat: add Frappe Charts integration examples (#546) by jooyoungseo · Pull Request #559 · xability/maidr

jooyoungseo · 2026-02-24T20:48:02Z

Add example HTML files demonstrating how to use MAIDR with Frappe Charts,
a lightweight open-source SVG charting library. Each example includes a
binder/adapter pattern that extracts data from Frappe Charts instances
and constructs the MAIDR JSON schema with appropriate CSS selectors for
SVG highlighting.

Supported chart types:

Bar chart (frappe-bar.html)
Line chart (frappe-line.html)
Scatter/dot plot (frappe-scatter.html)
Mixed axis chart with bar + line layers (frappe-mixed.html)

https://claude.ai/code/session_01LpiQt2NvNwqEsNUo3sPTpg

Add example HTML files demonstrating how to use MAIDR with Frappe Charts, a lightweight open-source SVG charting library. Each example includes a binder/adapter pattern that extracts data from Frappe Charts instances and constructs the MAIDR JSON schema with appropriate CSS selectors for SVG highlighting. Supported chart types: - Bar chart (frappe-bar.html) - Line chart (frappe-line.html) - Scatter/dot plot (frappe-scatter.html) - Mixed axis chart with bar + line layers (frappe-mixed.html) https://claude.ai/code/session_01LpiQt2NvNwqEsNUo3sPTpg

https://claude.ai/code/session_01LpiQt2NvNwqEsNUo3sPTpg

claude · 2026-02-24T20:50:09Z

Code Review

This PR adds four HTML example files demonstrating MAIDR integration with Frappe Charts. The adapter/binder pattern is well-conceived and the code is clearly commented. Here are my findings:

Positive aspects

Consistent integration pattern across all four chart types
Well-commented code that explains each step of the adapter process
Proper null check for the SVG element before proceeding
Covers a useful range: bar, line, scatter, and mixed chart types

Issues

1. Fragile timing with `setTimeout(100)`

All four files use setTimeout(() => {}, 100) with a comment that Frappe Charts "renders synchronously". If it truly renders synchronously, this fixed 100ms delay is both unnecessary and fragile—it will be too slow on fast machines and potentially too short on slow devices or under load.

Recommended approach:

// If synchronous rendering is guaranteed, use rAF for one paint cycle
requestAnimationFrame(() => {
  const svg = container.querySelector('svg.frappe-chart');
  // ...
});

If there's a genuine async concern, consider polling with a MutationObserver or using Frappe Charts' callback API if available.

2. CSS selectors are tightly coupled to Frappe Charts internals (frappe-bar.html, frappe-line.html, frappe-mixed.html, frappe-scatter.html)

The selectors like .dataset-units.dataset-bars rect.bar and .dataset-units.dataset-line circle depend on Frappe Charts' internal DOM structure. These could silently break if Frappe Charts changes class names in a future version.

Suggest adding a comment explicitly noting this dependency:

// These selectors are tied to frappe-charts@1.6.2 internal DOM structure.
// Verify selectors if upgrading the library version.
const barSelector = '.dataset-units.dataset-bars rect.bar';

3. Inconsistent selector specificity in frappe-scatter.html

In frappe-scatter.html, the selector is .dataset-units circle (no .dataset-line filter), while frappe-line.html uses the more specific .dataset-units.dataset-line circle. The comment in frappe-scatter.html even notes that Frappe Charts renders scatter points inside a dataset-line group, making this inconsistency confusing. If a multi-dataset scatter chart were used, the broader selector could match unintended elements.

Recommend aligning with frappe-line.html:

const pointSelector = '.dataset-units.dataset-line circle';

4. Scatter chart represents pseudo-scatter data (frappe-scatter.html)

Frappe Charts' scatter type places x-axis points at evenly spaced intervals from labels, regardless of their numeric values. So although the MAIDR data correctly stores { x: 10, y: 22 }, { x: 20, y: 25 }, ..., the visual SVG positions these at equal intervals—not at x-positions proportional to the label values. Users learning from this example may not realize the visual chart doesn't reflect true scatter geometry.

Consider adding a comment:

// Note: Frappe Charts' scatter type spaces labels evenly on the x-axis,
// regardless of their numeric values. MAIDR data uses the correct numeric
// values, so the sonification and text outputs are accurate, but the visual
// x-spacing does not reflect actual data distances.

5. Missing semantic HTML landmarks

For a project focused on accessibility, the example pages should model good HTML structure. None of the four files wrap their content in a <main> landmark:

<body>
  <main>
    <h1>Frappe Charts Bar Chart with MAIDR</h1>
    <div id="chart"></div>
  </main>
  <script>...</script>
</body>

6. No error feedback if CDN scripts fail to load

If the Frappe Charts CDN is unavailable, users see a blank page with no indication of what went wrong. A simple onerror handler on the <script> tag in <head> would improve the developer experience:

<script 
  src="https://cdn.jsdelivr.net/npm/frappe-charts@1.6.2/dist/frappe-charts.min.iife.js"
  onerror="document.body.textContent = 'Failed to load Frappe Charts from CDN.'">
</script>

7. Minor: version bump belongs in a separate commit

The package.json/package-lock.json version bump (3.50.0 → 3.51.0) is included in this PR. If this bump is intentional to mark the addition of these examples, it is fine—but version bumps are easier to reason about when isolated in their own commit so release tracking is clearer.

Summary

The integration pattern is solid and the examples will be valuable for developers wanting to use MAIDR with Frappe Charts. The main items worth addressing before merging are the fragile setTimeout timing (#1), the selector inconsistency in the scatter example (#3), and the missing <main> landmark (#5) given this project's accessibility focus.

Address bot code review feedback: - Replace setTimeout(100) with requestAnimationFrame() for reliable render timing - Add <main> semantic landmark for accessibility - Add CDN error handling with onerror and fallback message - Add version comments noting CSS selector dependency on Frappe Charts v1.6.2 - Fix scatter selector to use specific .dataset-line class instead of broad .dataset-units circle - Add comment about Frappe Charts' pseudo-scatter x-axis spacing - Fix line/mixed chart selectors to use path selector (.line-graph-path) as required by MAIDR's LineTrace (expects SVG path, not circles) Add new examples for remaining compatible MAIDR chart types: - Stacked bar (frappe-stacked-bar.html): Frappe barOptions.stacked → MAIDR stacked_bar with SegmentedPoint data - Multi-line (frappe-multiline.html): Multiple Frappe datasets → MAIDR line with per-dataset path selectors and legend MAIDR chart types not supported by Frappe Charts: box, candlestick, dodged_bar, heat (calendar vs grid format), hist, smooth, stacked_normalized_bar. https://claude.ai/code/session_01LpiQt2NvNwqEsNUo3sPTpg

claude · 2026-02-25T02:19:58Z

PR Review: Frappe Charts Integration Examples

Overall, this is a well-structured and useful addition. The adapter/binder pattern is clearly explained, and the CSS selector comments with version pins are especially helpful for maintainability. A few items worth addressing:

Issues

1. Unrelated version bump in `package-lock.json` (minor but clean up)

The package-lock.json bump from 3.50.0 → 3.51.0 appears unrelated to this feature. If a version bump is intentional, it should be in a dedicated commit or explained in the PR description. Otherwise, revert to avoid mixing concerns.

2. `requestAnimationFrame` timing may be fragile on slow devices

All five files use a single requestAnimationFrame to wait for Frappe Charts to finish rendering. Frappe Charts renders synchronously, so one frame is usually sufficient — but on a very slow device or under heavy CPU load, the DOM might not be fully ready. The existing comment is helpful, but consider adding a DOM-presence fallback:

requestAnimationFrame(() => {
  const svg = container.querySelector('svg.frappe-chart');
  if (!svg) {
    // Frappe Charts may not have rendered yet (slow device).
    // A MutationObserver or a second rAF could make this more robust.
    console.error('Frappe Charts SVG not found after one frame');
    return;
  }
  // ...
});

At minimum, the existing console.error is good — just make sure it's clear to users what to try (e.g., wrap in another rAF).

3. Empty `if` branch is misleading (`frappe-bar.html`, all files)

if (typeof frappe === 'undefined') {
  // Frappe Charts failed to load; bail out gracefully.
} else {
  // ... all the actual logic
}

The empty if body is a code smell. Invert the condition for clarity and remove the empty branch:

if (typeof frappe === 'undefined') {
  // Error message is already shown by onerror on the <script> tag.
  return; // or just let it fall through with no else
}
// ... all the actual logic (un-nested)

This also reduces the indentation level of the entire script block.

Security

4. No Subresource Integrity (SRI) on CDN resources

<script src="https://cdn.jsdelivr.net/npm/frappe-charts@1.6.2/dist/frappe-charts.min.iife.js"></script>

CDN-loaded assets should include an integrity attribute to guard against CDN compromise. You can generate SRI hashes at https://www.srihash.org/ or with openssl:

<script
  src="https://cdn.jsdelivr.net/npm/frappe-charts@1.6.2/dist/frappe-charts.min.iife.js"
  integrity="sha384-<HASH>"
  crossorigin="anonymous"
  onerror="document.getElementById('error').hidden=false"
></script>

The version is pinned to 1.6.2 (good!), but SRI provides the actual content guarantee.

Observations / Suggestions

5. `frappe-stacked-bar.html`: `fill` field uses dataset names — verify MAIDR's SegmentedTrace expectation

The existing stacked bar example (stacked-barplot.html) uses "fill": "0" / "fill": "1" (positional string indices). The new file uses dataset names ("fill": "Product A"). This should work since MAIDR uses the fill value as a display label, but it's worth confirming against the SegmentedTrace implementation that string fill values produce the expected text description output.

6. Missing `<meta name="viewport">`

All five new files omit:

<meta name="viewport" content="width=device-width, initial-scale=1" />

This is a minor accessibility/responsive design gap. Existing examples also miss it, so this is low priority — but worth fixing here since these are new files.

7. `frappe-scatter.html`: axis label mismatch in data vs axes

The scatter chart labels x as 'Altitude (m)' in axes but the data labels are [10, 20, ..., 100] representing altitude values — which is correct. The comment explains the Frappe spacing quirk well. Consider also noting that the x-axis in MAIDR will read the numeric label (not the axis title) since Frappe treats x positions as categorical.

8. Dynamic maidr.js loading — document this as the required pattern for third-party libs

The existing examples use <script src="../dist/maidr.js"> in <head> (synchronous, before the SVG). These new examples dynamically inject the script after chart render. This is the correct approach for third-party chart libraries that need to render before MAIDR attaches. A brief comment like:

// maidr.js must be loaded AFTER the SVG is ready and maidr-data is set.
// Dynamic injection ensures this ordering when using third-party chart libraries.

...would help future contributors understand why this differs from the static pattern.

Summary

Category	Finding	Severity
Code quality	Empty `if` branch; invert condition	Low
Reliability	Single `rAF` may miss on slow devices	Low
Security	Missing SRI hashes on CDN resources	Medium
Correctness	`fill` value format in stacked bar — verify	Low
Style	Missing viewport meta	Trivial
Unrelated change	`package-lock.json` version bump	Low

The examples are well-commented and demonstrate the integration pattern clearly. Addressing the SRI hashes and the empty if branch would be the highest-priority changes before merging.

- Invert empty if-branch: use `if (typeof frappe !== 'undefined')` guard instead of empty-body-with-else pattern - Add retry loop: named requestAnimationFrame callback retries if SVG is not yet in DOM, handling slow device rendering - Add SRI hashes: integrity and crossorigin attributes on CDN script and link tags for supply-chain security - Add viewport meta: responsive design support via <meta name="viewport"> on all example files - Document dynamic loading: explain why maidr.js is loaded dynamically instead of statically (Frappe Charts generates SVG at runtime) - Clarify stacked bar fill: add comment noting that SegmentedTrace accepts both descriptive names and positional indices for fill field https://claude.ai/code/session_01LpiQt2NvNwqEsNUo3sPTpg

claude · 2026-02-25T22:26:48Z

PR Review: Frappe Charts Integration Examples

Thanks for adding these examples — the adapter/binder pattern is well-structured and the inline documentation explaining Frappe Charts' DOM structure is genuinely helpful for anyone trying to integrate other libraries.

Positive Highlights

SRI hashes on all CDN resources is good security practice.
Version-pinned CDN (frappe-charts@1.6.2) with a clear comment warning about upgrading and verifying CSS selectors.
Consistent pattern across all files: create chart → wait for DOM → build MAIDR schema → inject script.
CDN failure fallback via onerror and the guard if (typeof frappe !== 'undefined').
Correct MAIDR type identifiers: 'point' for scatter (TraceType.SCATTER = 'point'), 'stacked_bar' for stacked bars (TraceType.STACKED = 'stacked_bar').
Dynamic lineSelectors in frappe-multiline.html — cleanly handles N datasets without hardcoding.

Issues to Address

1. PR Description Does Not Match Added Files (Minor)

The PR description lists 4 chart types, but 6 files are added. frappe-multiline.html and frappe-stacked-bar.html are not mentioned. Please update the PR description to list all supported chart types.

2. Unbounded `requestAnimationFrame` Retry Loop (Medium)

In all 6 files:

requestAnimationFrame(function initMaidr() {
  const svg = container.querySelector('svg.frappe-chart');
  if (!svg) {
    requestAnimationFrame(initMaidr); // ← no max retry count
    return;
  }
  // ...
});

If the SVG is never rendered (e.g., a silent Frappe Charts error after loading), this retries indefinitely. The if (typeof frappe !== 'undefined') guard handles CDN failure, but not silent render failures. Consider adding a retry cap:

requestAnimationFrame(function initMaidr(retries = 0) {
  const svg = container.querySelector('svg.frappe-chart');
  if (!svg) {
    if (retries < 20) requestAnimationFrame(() => initMaidr(retries + 1));
    return;
  }
  // ...
});

3. Potential Null Dereference on `container` (Low)

const container = document.querySelector('#chart');
const svg = container.querySelector('svg.frappe-chart'); // throws if container is null

container will never be null in these specific examples (the <div id="chart"> is always in the HTML), but a null check would make the pattern safer to copy-paste into other contexts:

const container = document.querySelector('#chart');
if (!container) return;

4. Inconsistent `title` Field on Layers (Minor)

frappe-mixed.html includes title on each layer ('Sales (Bar)', 'Trend (Line)'), but the other examples omit it. If title improves accessibility output for multi-layer charts, it should be documented as intentional. If it's required for consistent behaviour across chart types, consider adding it to the single-layer examples too.

5. Stale Comment References `groupDirection` (Minor)

In frappe-stacked-bar.html:

// Setting groupDirection to 'forward' aligns the mapping.

No groupDirection property appears in the MAIDR schema constructed below. This comment is misleading — either the property was removed or this is a leftover from an earlier implementation. Please remove or correct it.

6. Version Bump in `package-lock.json` Only (Minor)

The lock file bumps from 3.50.0 → 3.51.0, but package.json does not appear to be part of this PR's file list. Typically both files should be updated together. If this is intentional (e.g., a separate versioning PR), it's worth noting in the description.

Accessibility Note

The <div id="chart"> container has no accessible label before MAIDR initialises. Sighted users see the chart rendering; screen reader users get nothing until MAIDR loads. Consider adding a temporary loading state or aria-label on the container that is removed once MAIDR attaches:

<div id="chart" aria-label="Chart loading..."></div>

Summary

Category	Status
Code quality	✅ Good — consistent, readable, well-commented
Security	✅ SRI hashes present
Correctness	⚠️ Unbounded retry loop; stale comment
Accessibility	⚠️ No loading state before MAIDR init
PR completeness	⚠️ Description doesn't cover all 6 files; version bump in lock only
Tests	N/A — example files; no automated tests expected

The core pattern is solid and well-suited for copying into real projects. Addressing the retry cap and the stale comment would be the most impactful changes before merging.

claude added 2 commits February 24, 2026 20:41

chore: sync package-lock.json version to 3.51.0

a776c01

https://claude.ai/code/session_01LpiQt2NvNwqEsNUo3sPTpg

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add Frappe Charts integration examples (#546)#559

feat: add Frappe Charts integration examples (#546)#559
jooyoungseo wants to merge 4 commits into
mainfrom
claude/fix-issue-546-26Cn3

jooyoungseo commented Feb 24, 2026

Uh oh!

claude Bot commented Feb 24, 2026

Uh oh!

claude Bot commented Feb 25, 2026

Uh oh!

claude Bot commented Feb 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jooyoungseo commented Feb 24, 2026

Uh oh!

claude Bot commented Feb 24, 2026

Code Review

Positive aspects

Issues

1. Fragile timing with setTimeout(100)

2. CSS selectors are tightly coupled to Frappe Charts internals (frappe-bar.html, frappe-line.html, frappe-mixed.html, frappe-scatter.html)

3. Inconsistent selector specificity in frappe-scatter.html

4. Scatter chart represents pseudo-scatter data (frappe-scatter.html)

5. Missing semantic HTML landmarks

6. No error feedback if CDN scripts fail to load

7. Minor: version bump belongs in a separate commit

Summary

Uh oh!

claude Bot commented Feb 25, 2026

PR Review: Frappe Charts Integration Examples

Issues

1. Unrelated version bump in package-lock.json (minor but clean up)

2. requestAnimationFrame timing may be fragile on slow devices

3. Empty if branch is misleading (frappe-bar.html, all files)

Security

4. No Subresource Integrity (SRI) on CDN resources

Observations / Suggestions

5. frappe-stacked-bar.html: fill field uses dataset names — verify MAIDR's SegmentedTrace expectation

6. Missing <meta name="viewport">

7. frappe-scatter.html: axis label mismatch in data vs axes

8. Dynamic maidr.js loading — document this as the required pattern for third-party libs

Summary

Uh oh!

claude Bot commented Feb 25, 2026

PR Review: Frappe Charts Integration Examples

Positive Highlights

Issues to Address

1. PR Description Does Not Match Added Files (Minor)

2. Unbounded requestAnimationFrame Retry Loop (Medium)

3. Potential Null Dereference on container (Low)

4. Inconsistent title Field on Layers (Minor)

5. Stale Comment References groupDirection (Minor)

6. Version Bump in package-lock.json Only (Minor)

Accessibility Note

Summary

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

1. Fragile timing with `setTimeout(100)`

1. Unrelated version bump in `package-lock.json` (minor but clean up)

2. `requestAnimationFrame` timing may be fragile on slow devices

3. Empty `if` branch is misleading (`frappe-bar.html`, all files)

5. `frappe-stacked-bar.html`: `fill` field uses dataset names — verify MAIDR's SegmentedTrace expectation

6. Missing `<meta name="viewport">`

7. `frappe-scatter.html`: axis label mismatch in data vs axes

2. Unbounded `requestAnimationFrame` Retry Loop (Medium)

3. Potential Null Dereference on `container` (Low)

4. Inconsistent `title` Field on Layers (Minor)

5. Stale Comment References `groupDirection` (Minor)

6. Version Bump in `package-lock.json` Only (Minor)