Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion src/explore-education-statistics-common/package.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, could you put these in the right place (alphabetical ordered)? Will just help when scanning/managing deps as there are many of them and is standard practise. Ta!

Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@
"use-immer": "^0.9.0",
"uuid": "^8.3.1",
"xlsx": "^0.17.4",
"yup": "^1.3.3"
"yup": "^1.3.3",
"color-namer": "^1.4.0"
},
"devDependencies": {
"@swc/core": "^1.4.17",
Expand All @@ -70,6 +71,7 @@
"@types/react-modal": "^3.16.0",
"@types/sanitize-html": "^2.9.0",
"@types/uuid": "^8.3.0",
"@types/color-namer": "^1.3.3",
"eslint": "^8.57.0",
"intersection-observer": "^0.11.0",
"jest": "^29.6.1",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,27 @@
margin-bottom: 0;
}

.legend dd {
margin: 0;
padding: 0;
}

.legendIcon {
border: 1px solid govuk-colour('black');
forced-color-adjust: none;
height: 20px;
margin-right: govuk-spacing(1);
width: 20px;
}

.srOnly {
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't need to define this class here - we already have visually hidden text as a component you can use instead of a span className="srOnly"

<VisuallyHidden>text here</VisuallyHidden>

position: absolute;
width: 1px;
height: 1px;
margin: -1px;
padding: 0;
border: 0;
overflow: hidden;
clip-path: inset(100%);
white-space: nowrap;
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import { Dictionary } from '@common/types';
import classNames from 'classnames';
import { Feature, FeatureCollection, Geometry } from 'geojson';
import { Layer, Path, Polyline } from 'leaflet';
import type { LeafletEvent, Map as LeafletMap } from 'leaflet';
import keyBy from 'lodash/keyBy';
import React, { useCallback, useEffect, useMemo, useState } from 'react';
import { MapContainer } from 'react-leaflet';
Expand Down Expand Up @@ -115,6 +116,7 @@ export default function MapBlock({
dataGroups: deprecatedDataGroups,
dataClassification: deprecatedDataClassification,
data,
alt,
map,
meta,
legend,
Expand Down Expand Up @@ -296,6 +298,15 @@ export default function MapBlock({
center={position}
minZoom={5}
zoom={5}
// @ts-expect-error The library's type definition is incorrect.
whenReady={(e: LeafletEvent) => {
const mapContainer = (e.target as LeafletMap).getContainer();
const altText = alt
? `${alt}, for alternative see table tab`
: 'Interactive map showing education statistics by area, for alternative see table tab';
mapContainer?.setAttribute('aria-label', altText);
mapContainer?.setAttribute('role', 'group');
}}
>
<MapGeoJSON
dataSetCategoryConfigs={dataSetCategoryConfigs}
Expand Down
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking you've ran the UI tests after this change? I know we're querying for legend items - hopefully they shouldn't break because of it but just want to make sure

Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import styles from '@common/modules/charts/components/MapBlock.module.scss';
import { LegendDataGroup } from '@common/modules/charts/components/utils/generateLegendDataGroups';
import React from 'react';
import ColorNamer from 'color-namer';

interface Props {
heading: string;
Expand All @@ -13,24 +14,75 @@ export default function MapLegend({ heading, legendDataGroups }: Props) {
<h3 className="govuk-heading-s dfe-word-break--break-word">
Key to {heading}
</h3>
<ul className="govuk-list" data-testid="mapBlock-legend">
<dl className="govuk-list" data-testid="mapBlock-legend">
Copy link
Contributor

Choose a reason for hiding this comment

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

After converting to the <dl> we're now missing margin beneath each item - it was applied to li elements, can be added in manually to our styles.legend class.

image

{legendDataGroups.map(({ min, max, colour }) => (
<li
<div
key={`${min}-${max}-${colour}`}
className={styles.legend}
data-testid="mapBlock-legend-item"
>
<span
className={styles.legendIcon}
data-testid="mapBlock-legend-colour"
style={{
backgroundColor: colour,
}}
/>
{`${min} to ${max}`}
</li>
<dt>
<div
className={styles.legendIcon}
data-testid="mapBlock-legend-colour"
style={{
backgroundColor: colour,
}}
/>
</dt>
<span className={styles.srOnly}>
Copy link
Contributor

Choose a reason for hiding this comment

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

2 points:

  • This span (change to VisuallyHidden as per above comment) needs to go inside the <dt>
  • I don't think we need the colour to be written in hex format particularly, the important thing is to write the value of the colour so it is not only visually represented. So probably fine to leave in rgb format if that's what it's saved/returned as.

map colour {rgbaToHex(colour)} ({hexToColourName(colour)})
</span>
<dd>{`${min} to ${max}`}</dd>
</div>
))}
</ul>
</dl>
</>
);
}

function hexToColourName(rgba: string): string | false {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think hexToColourName is the right name if we're converting from rgba!

const name = ColorNamer(rgba, { pick: ['ntc', 'pantone'] });
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of interest, any reason we're picking these two lists?
NIT but the variable could be called names as it's got multiple properties/lists

const pantone = name.pantone[0] ? name.pantone[0].name : false;
const ntc = name.ntc[0] ? name.ntc[0].name : false;
if (ntc) return ntc;
if (pantone) return pantone;
return false;
}
Comment on lines +46 to +51
Copy link
Contributor

Choose a reason for hiding this comment

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

Think we could make this a bit simpler instead of explicitly passing false everywhere e.g.
image


function componentToHex(c: number): string {
const hex = c.toString(16);
return hex.length === 1 ? `0${hex}` : hex;
}

function rgbaToHex(rgba: string): string {
const match = rgba.match(
/^rgba?\(\s*(\d{1,3})\s*,\s*(\d{1,3})\s*,\s*(\d{1,3})(?:\s*,\s*([\d.]+))?\s*\)$/,
);
if (!match) {
throw new Error('Invalid RGBA color format');
}

const [, r, g, b, a] = match;
const rParsed = parseInt(r, 10);
const gParsed = parseInt(g, 10);
const bParsed = parseInt(b, 10);

const clamp = (n: number, min: number, max: number) =>
Math.min(max, Math.max(min, n));

const red = clamp(Math.round(rParsed), 0, 255);
const green = clamp(Math.round(gParsed), 0, 255);
const blue = clamp(Math.round(bParsed), 0, 255);

let alphaHex = '';

if (a !== undefined) {
const alpha = clamp(parseFloat(a), 0, 1);
alphaHex = componentToHex(Math.round(alpha * 255));
}

return `#${componentToHex(red)}${componentToHex(green)}${componentToHex(
blue,
)}${alphaHex}`;
}
Comment on lines +53 to +88
Copy link
Contributor

Choose a reason for hiding this comment

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

See above comment, but I don't think we need these two functions

Original file line number Diff line number Diff line change
Expand Up @@ -383,4 +383,60 @@ describe('MapBlock', () => {
expect(legendColours[3].style.backgroundColor).toBe('rgb(108, 130, 183)');
expect(legendColours[4].style.backgroundColor).toBe('rgb(71, 99, 165)');
});

describe('MapBlock (aria-label)', () => {
const baseProps: MapBlockProps = {
...testMapConfiguration,
boundaryLevel: 1,
id: 'testMapAria',
axes: testMapConfiguration.axes as MapBlockProps['axes'],
legend: testMapConfiguration.legend as LegendConfiguration,
meta: testFullTable.subjectMeta,
data: testFullTable.results,
height: 600,
width: 900,
onBoundaryLevelChange,
};

test('sets aria-label to "Interactive map showing education statistics by area" on the map container when alt is not provided', async () => {
const { container } = render(<MapBlock {...baseProps} />);

await waitFor(() => {
expect(
screen.getByLabelText('1. Select data to view'),
).toBeInTheDocument();
});

const mapEl = container.querySelector<HTMLElement>('.leaflet-container');
expect(mapEl).not.toBeNull();

await waitFor(() => {
expect(mapEl).toHaveAttribute(
'aria-label',
'Interactive map showing education statistics by area, for alternative see table tab',
);
});
});

test('sets aria-label on both wrapper and map container when alt is provided', async () => {
const altText = 'Pupils absence map';
const { container } = render(<MapBlock {...baseProps} alt={altText} />);

await waitFor(() => {
expect(
screen.getByLabelText('1. Select data to view'),
).toBeInTheDocument();
});

const mapEl = container.querySelector<HTMLElement>('.leaflet-container');
expect(mapEl).not.toBeNull();

await waitFor(() => {
expect(mapEl).toHaveAttribute(
'aria-label',
`${altText}, for alternative see table tab`,
);
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ describe('MapLegend', () => {
}),
).toBeInTheDocument();

const listItems = screen.getAllByRole('listitem');
const listItems = screen.getAllByRole('definition');
expect(listItems).toHaveLength(2);
expect(listItems[0]).toHaveTextContent('1 to 3');
expect(listItems[1]).toHaveTextContent('4 to 5');
Expand All @@ -48,4 +48,18 @@ describe('MapLegend', () => {
expect(legendColours[0].style.backgroundColor).toBe('rgb(128, 128, 128)');
expect(legendColours[1].style.backgroundColor).toBe('rgb(0, 0, 0)');
});

test('renders a span with hex and colour name for each legend item', () => {
render(
<MapLegend
heading="Test heading"
legendDataGroups={testLegendDataGroups}
/>,
);

const spans = screen.getAllByTestId('mapBlock-legend-item');
expect(spans).toHaveLength(2);
expect(spans[0]).toHaveTextContent('map colour #808080 (Gray)');
expect(spans[1]).toHaveTextContent('map colour #000000 (Black)');
});
});