Skip to content

only show summary button if there is a summary#139

Closed
markconroy wants to merge 9 commits into2.xfrom
feature/138-summaries-buttons
Closed

only show summary button if there is a summary#139
markconroy wants to merge 9 commits into2.xfrom
feature/138-summaries-buttons

Conversation

@markconroy
Copy link
Member

Closes #138

What does this change?

  1. Checks the there is a value in the summary field before showing the "Show summary" button.
  2. Checks that there is at least one summary before showing the "Show all summaries" button

Thanks to Big Blue Door for sponsoring my time to work on this.

@finnlewis
Copy link
Member

We probably want to look at this together with #131

Either merge this first, then incorporate this into #131 .. or just do the latter.

@tonypaulbarker
Copy link

tonypaulbarker commented Nov 18, 2025

Manual testing passes but we need to address failing tests.

@markconroy
Copy link
Member Author

@tonypaulbarker Those failing tests are already present, and not caused by this PR.

Let's get this merged and file a follow up to fix the failing tests - convert var to const, etc.

@tonypaulbarker tonypaulbarker self-requested a review November 23, 2025 23:35
@tonypaulbarker
Copy link

The PHP Unit tests were passing in the last release and ES Lint was passing in https://github.com/localgovdrupal/localgov_step_by_step/actions/runs/19265037637

The ESLint test notice line numbers refer to the code changes here.
PHP Unit tests complains that the summary isn't showing so needs review to find the cause and perhaps updating to handle this case.

@markconroy
Copy link
Member Author

Ooops, sorry @tonypaulbarker

The JS items are fixed up now. I'll check the PHP ones once the tests have finished running.

Copy link
Member

@finnlewis finnlewis left a comment

Choose a reason for hiding this comment

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

Discussing in Merge Tuesday, @markconroy has fixed the tests now and @tonypaulbarker approved the manual testing last week.

However, @markconroy just tested manually on a screen share, and the show all is not working as expected.

Needs a little more work!

@finnlewis finnlewis self-requested a review November 25, 2025 12:36
@markconroy
Copy link
Member Author

All working now!!!

@finnlewis
Copy link
Member

Almost !

Testing locally, when showing all items individually, showing the last item changes the global toggle to "hide summaries'

But clicking on it the first time does not hide.

Clicking the second time does hide all the summaries.

So need to make sure the aria-expanded attribute is set to true once the last summary is shown.

@finnlewis
Copy link
Member

@finnlewis finnlewis closed this Dec 9, 2025
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.

Don't show 'Show step summary' button if there is no summary

3 participants