[DTOSS-12327] - Render image markers on breast features diagram by colinrotherham · Pull Request #1100 · NHSDigital/dtos-manage-breast-screening

colinrotherham · 2026-03-03T09:28:18Z

Description

This PR uses the persisted region X/Y data to render image markers

Breast.features.diagram.mp4

Jira link

https://nhsd-jira.digital.nhs.uk/browse/DTOSS-12327

Review notes

In draft whilst I sort out some failing tests

Review checklist

Check database queries are correctly scoped to current_provider

github-actions · 2026-03-03T12:49:22Z

The review app at this URL has been deleted:
https://pr-1100.manage-breast-screening.non-live.screening.nhs.uk

MatMoore · 2026-03-04T10:46:20Z

manage_breast_screening/assets/js/image-map.js

-    this.$image.addEventListener('mousemove', this.onMouseMove.bind(this))
-    this.$image.addEventListener('mouseleave', this.onMouseLeave.bind(this))
-    this.$image.addEventListener('click', this.onClick.bind(this))
+    if (!readOnly) {


do we have a unit test for this case?

MatMoore · 2026-03-04T11:19:34Z

manage_breast_screening/assets/js/breast-diagram.js

+      const point = this.$imageMap.createPoint(x, y, id)
+      const region = this.$imageMap.createRegion($path, point)
+
+      this.setMarker(region, index)


Is features still the right name for this, now that we have markers as well? They seem like different representations of the same thing.

Also, having all these side effects inside a getter feels like a code smell to me. Perhaps we could turn features into an attribute and then onUpdate calls a method to recompute it?

Agree entirely

I've pushed up a change to use event listeners

Breast diagram listens via addEventListener()

Breast diagram handles this.onClick() when image map dispatches click

Breast diagram calls this.add() to modify values

I've temporarily added 4124fa2 so you can see this.remove() in action too

MatMoore · 2026-03-04T11:54:50Z

manage_breast_screening/assets/js/breast-diagram.js

-      this.$imageMap.setState('active', feature.region, null)
+    const { $imageMap, features, markers } = this
+
+    // Remove excess markers


I'm finding this code quite difficult to follow.

From the name, I expected render() to be called any time something changes, but actually it's just called one time as part of initialising the diagram.

It looks almost as if markers should be empty at this point, but it's populated as a side effect of accessing this.features. Then we are removing some markers from $root but it's not clear why we would have extra markers at this point.

I think this complexity comes from having to keep in sync multiple different things, and having to do that throughout the whole class. To reason about the correctness of the code you have to consider:

the values array

the features array

the image map's state (a region is active or not active)

the markers array

whether the marker is added to $root or not

I don't know if there's a way we could either reduce the amount of state, or encapsulate the state changes in add/update/remove methods so that these things all change at the same time?

Love this feedback, you're right

My latest push makes sure:

Image map only needs to worry about regions (SVG paths)

Feature diagram only needs to worry about markers

Regarding this bit:

// Remove excess markers for (const marker of markers.splice(values.length)) { marker.$root.remove() }

I've used a similar technique to particle systems in games, to keep memory usage down

Rather than create, append and position every marker on every render, I've reused them

Similarly when features are removed from the diagram values.length is reduced, so we .remove() unused markers over that length rather than clearing and creating them all again

MatMoore · 2026-03-04T12:02:00Z

manage_breast_screening/assets/js/image-marker.js

+   * @param {Partial<ImageMarkerConfig>} [config] - Image marker config
+   */
+  constructor($root, config = {}) {
+    $root = $root ?? document.createElement(config.href ? 'a' : 'div')


Maybe worth adding a comment to explain that div is for the readonly version. Sidenote: is <div> ok to use here or should it be <g> or something specific to svg?

Yeah good point

Are we still happy with the proposal to use <div> for readonly and ~~<a>~~ <button> otherwise?

MatMoore · 2026-03-04T12:05:11Z

manage_breast_screening/assets/js/image-marker.js

+   * @param {DOMPoint} point - SVG point at pointer coordinates
+   */
+  set point(point) {
+    const offsetX = Math.min(Math.max(point.x, 20), this.config.width - 20)


what's the significance of 20 here? We're bounding the point to within 20px of the border?

20px is the width of the gutter

I've split out the code and added some comments in 5fb6b76

i.e. All clicks within the gutter are rounded 20px away from the image map edges

It makes sure we don't lose the makers near the edges:

MatMoore · 2026-03-04T12:08:56Z

manage_breast_screening/assets/sass/components/_breast-diagram.scss

+  cursor: crosshair;
+}
+
+input[readonly] ~ .app-breast-diagram__image-map .app-breast-diagram__svg {


Possibly worth creating a specific class like app-breast-diagram__image-map--readonly rather than depending strictly on the html structure for this.

Addressed in ecc909f and f3a0be0

MatMoore

Sorry for the slow reply. I think it looks a lot clearer now post-refactor. Lets merge (once the temporary commit is removed) 🚢

I guess next part is the selection of feature type and the legend on the right?

In the meantime I'll finish up #1196

MatMoore · 2026-03-19T17:16:29Z

...breast_screening/mammograms/jinja2/mammograms/medical_information/breast_features/form.jinja

+  "method": "post",
+  "data-module": "app-breast-diagram",
+  "data-debug": {
+    "value": "true" if "debug" in request.GET else false,


you've got a mix of string and bool here. If the intention is for value to be boolean you can simplify the expression to "debug" in request.GET

Yeah it's intentional

We need an actual attribute value for the component config to pick up

~~When true it appends data-debug~~

When "true" it appends data-debug="true"

When false it skips the attribute

Ah got it, all good then 👍🏻

colinrotherham · 2026-03-19T18:17:51Z

Thanks @MatMoore

You'll see these two variables in mammograms/medical_information/breast_features/form.jinja

{% set diagram_debug = "debug" in request.GET %}
{% set diagram_read_only = false %}

They become data-debug="true" and/or data-read-only="true" on app-breast-diagram

Ready for you to wire up in #1109

sonarqubecloud · 2026-03-19T18:36:12Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

colinrotherham · 2026-03-19T19:50:57Z

manage_breast_screening/assets/js/breast-diagram.js

+
+    if (!markers[index]) {
+      markers[index] = new ImageMarker(null, {
+        href: $input.readOnly ? undefined : `#marker-${index + 1}`,


We can address where the marker links to (or if we use a button) when adding the form fields

colinrotherham added the deploy label Mar 3, 2026

colinrotherham had a problem deploying to review March 3, 2026 09:30 — with GitHub Actions Failure

colinrotherham force-pushed the DTOSS-12327-render-markers branch from 03160d6 to 8d89d00 Compare March 3, 2026 12:43

colinrotherham temporarily deployed to review March 3, 2026 12:45 — with GitHub Actions Inactive

colinrotherham marked this pull request as ready for review March 3, 2026 12:52

colinrotherham requested a review from MatMoore March 3, 2026 12:52

MatMoore reviewed Mar 4, 2026

View reviewed changes

MatMoore mentioned this pull request Mar 4, 2026

[wip] add support for readonly breast diagram #1109

Draft

1 task

colinrotherham force-pushed the DTOSS-12327-render-markers branch from 8d89d00 to 4124fa2 Compare March 18, 2026 08:42

colinrotherham temporarily deployed to review March 18, 2026 08:44 — with GitHub Actions Inactive

MatMoore approved these changes Mar 18, 2026

View reviewed changes

colinrotherham added 11 commits March 19, 2026 16:22

Configure ESLint to allow optional catch binding

e75c0ab

Configure image map CSS class

27c44a6

Configure image map when readonly

ca9f6e8

Render image markers

1bd008f

Mock more image map methods

c043c07

Fix checks for path class

3fa8417

Clarify safe area calculation

76dd6af

Simplify image map to use events

727823a

Move image map highlight to CSS

5d86b3b

Bring back hover debug information

a2f6f79

Add config option for debug: true

44b42a1

colinrotherham force-pushed the DTOSS-12327-render-markers branch from 4124fa2 to 438cc69 Compare March 19, 2026 16:55

colinrotherham temporarily deployed to review March 19, 2026 16:56 — with GitHub Actions Inactive

colinrotherham force-pushed the DTOSS-12327-render-markers branch from 438cc69 to f3a0be0 Compare March 19, 2026 16:58

colinrotherham temporarily deployed to review March 19, 2026 17:00 — with GitHub Actions Inactive

colinrotherham force-pushed the DTOSS-12327-render-markers branch from f3a0be0 to 62bec64 Compare March 19, 2026 17:07

colinrotherham temporarily deployed to review March 19, 2026 17:09 — with GitHub Actions Inactive

Pass config via attributes

51b8464

Suppress debug info when not enabled

3c8965f

MatMoore reviewed Mar 19, 2026

View reviewed changes

colinrotherham force-pushed the DTOSS-12327-render-markers branch from 62bec64 to 3c8965f Compare March 19, 2026 17:16

colinrotherham temporarily deployed to review March 19, 2026 17:17 — with GitHub Actions Inactive

MatMoore approved these changes Mar 19, 2026

View reviewed changes

colinrotherham added 2 commits March 19, 2026 17:46

Move debug info into breast diagram component

90b03f3

Hide image map X/Y and region hover info when read only

cedaf6b

colinrotherham temporarily deployed to review March 19, 2026 17:49 — with GitHub Actions Inactive

colinrotherham temporarily deployed to review March 19, 2026 18:16 — with GitHub Actions Inactive

colinrotherham temporarily deployed to review March 19, 2026 18:27 — with GitHub Actions Inactive

Move read only config to breast diagram

be7ebb1

colinrotherham force-pushed the DTOSS-12327-render-markers branch from dd6dbef to be7ebb1 Compare March 19, 2026 18:35

colinrotherham temporarily deployed to review March 19, 2026 18:36 — with GitHub Actions Inactive

colinrotherham commented Mar 19, 2026

View reviewed changes

colinrotherham merged commit 6c62d1c into main Mar 19, 2026
13 checks passed

colinrotherham deleted the DTOSS-12327-render-markers branch March 19, 2026 19:51

colinrotherham deployed to review March 19, 2026 19:51 — with GitHub Actions Active

Conversation

colinrotherham commented Mar 3, 2026

Description

Jira link

Review notes

Review checklist

Uh oh!

github-actions bot commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MatMoore left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colinrotherham commented Mar 19, 2026

Uh oh!

sonarqubecloud bot commented Mar 19, 2026

Quality Gate passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions bot commented Mar 3, 2026 •

edited

Loading