Skip to content

feature: Implemented the new KS4 chart interaction across both ks4hea…#241

Merged
vipinsamreddy merged 42 commits into
mainfrom
feature/new-button-design
Jun 19, 2026
Merged

feature: Implemented the new KS4 chart interaction across both ks4hea…#241
vipinsamreddy merged 42 commits into
mainfrom
feature/new-button-design

Conversation

@vipinsamreddy

@vipinsamreddy vipinsamreddy commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Description

Summary

This branch updates the KS4 and attendance chart experience to use a consistent toggle-based interaction, improves chart rendering
and tooltip behaviour, and aligns related UI tests with the new behaviour. It also adds a configurable review DB reset flag for
review deployments and introduces a scheduled infrastructure validation workflow.

What changed

  • Added reusable content toggle support with new tag helpers:

    • content-toggle
    • toggle-content
  • Introduced a generic chart-toggle.js for tab-based chart sections that need converting into a chart toggle.

  • Kept content-toggle.js for pages that already render toggle markup directly.

  • Updated KS4 headline measures, KS4 core subjects, and attendance pages to use the new chart/toggle interaction patterns.

  • Improved chart rendering in chart-factory.js, including:

    • tooltip styling and positioning
    • legend placement and sizing
    • hover behaviour
    • better handling for chart resizing and mobile/desktop interactions
  • Updated data-view-switcher.js so year-by-year charts recalculate their y-axis correctly when dropdown selections change.

  • Added styling updates in main.scss for toggles, legends, tooltips, and chart spacing.

  • Updated UI tests to match the new toggle-based chart behaviour and dynamic axis behaviour.

  • Added reset_review_db support to review deployment config so review DB reset/restore can be turned on or off via terraform/
    application/config/review.yml.

  • Added a new validate-infrastructure.yml GitHub Actions workflow for scheduled/manual infra validation.

  • Updated Makefile terraform plan targets to support detailed exit codes.

Why

  • Replaces inconsistent chart/tab interactions with a clearer toggle-based pattern.
  • Fixes chart layout, tooltip, legend, and hover issues across KS4 and attendance views.
  • Makes dropdown-driven chart updates behave correctly.
  • Reduces accidental review DB resets by making that behaviour configurable.
  • Adds automated infra validation outside application deploys.

Testing

  • UI tests were updated for the new chart toggle behaviour.
  • I have not run the full branch validation in this session.

Related Trello Ticket

https://trello.com/c/sc1nAzT0/831-spike-graph-changes-updates-to-tab-button-view

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed
  • Tested on [Environment: Dev/Staging/Local]

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have verified accessibility requirements are met

Screenshots (if applicable)

Add UI changes, logs, console outputs anything to help reviewers.

@vipinsamreddy vipinsamreddy added the deploy deploy application label Jun 3, 2026
@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown

Review app for PR 241 was deleted

Comment thread SAPSec.Web/AssetSrc/js/ks4-chart-toggle.js Outdated
Comment thread SAPSec.Web/AssetSrc/js/chart-toggle.js
Comment thread SAPSec.Web/AssetSrc/js/ks4-chart-toggle.js Outdated
@stusherwin

stusherwin commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

Hi @vipinsamreddy it seems that this is using JavaScript to combine the first two tabs together (3-year average and Year by year) into one (Charts), and switch between them using a toggle? I assumed this would be implemented a separate component that would sit inside a tab panel.

I can see a few issues with this approach:

  1. The current govuk Tabs component pushes location history when changing tabs, so previously when you swap between 3-year average and year-by-year it pushes the anchor to the browser history (e.g. changes the location from #english-language-three-year-average to #english-language-year-by-year), so the user can use the back button to switch to the previous tab. We lose this functionality with this implementation. I'm not sure how big of an issue that is, maybe it's fine.
  2. It's a bit like magic, the Razor code lays out the tabs one way, and then the JavaScript kicks in and restructures the tabs. From a maintenance perspective I would expect to be able to look at the server-side code and see the structure laid out as it appears on the page. This is a small point I think but small things like this add up over time to affect the maintainability overall
  3. It relies on the first two tabs being in the same place and having the same text content. This is the biggest issue for me - the order and placement and text content of the tabs should not affect the functionality. For example if UX decided that they wanted the Top performers tab to come first in the list, or that the text "3-year average" needs to be changed to "Three-year average", these should be very simple changes, and we should not expect the chart display functionality to break as a result. As it stands we would have to edit the JavaScript code to accommodate a change to the tab contents.
  4. It's very specific to this particular use case and not easy to reuse in other areas of the service. For instance if we wanted to use this toggle feature in a different page to toggle between two pieces of content we would have to pretty much write that functionality again from scratch.

A more reusable pattern could be something like a toggle component, probably a tag helper like the TabbedViewTagHelper that would sit inside one of the tabs, like this:

image

If we were to use it along with the new tabbed-view component from the components spike the code might look something like this:

// tabbed-view component from components spike
<tabbed-view>
        <tab-content id="charts" name="Charts">

            // new content-toggle component (could do with a better name)
           <content-toggle>
                <toggle-content id="three-year-average" name="3-year-average">
                    <p>Three year average chart goes here...</p>
                </toggle-content>
                <toggle-content id="year-by-year" name="Year by year">
                    <p>Year by year chart goes here...</p>
                </toggle-content>
           <content-toggle>

        </tab-content>
        <tab-content id="table" name="Table">
            <p>Table content goes here...</p>
        </tab-content>
        <tab-content id="top-performers" name="Top performers">
            <p>Top performers content goes here...</p>
        </tab-content>
</tabbed-view>

@github-actions

Copy link
Copy Markdown

Code Coverage Report: Only Changed Files listed

Package Coverage
Overall Coverage 🟢 77.52%

Minimum allowed coverage is 0%, this run produced 77.52%

@vipinsamreddy vipinsamreddy merged commit 3b898a5 into main Jun 19, 2026
7 checks passed
@vipinsamreddy vipinsamreddy deleted the feature/new-button-design branch June 19, 2026 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deploy deploy application

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants