- 
                Notifications
    
You must be signed in to change notification settings  - Fork 1
 
HEEDLS-871 Add category topic tags and fix incorrect behaviour #1098
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HEEDLS-871 Add category topic tags and fix incorrect behaviour #1098
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, couple of minor points.
Also I'm not sure Kevin's response gives a clear answer as to which filters we want to display, might be worth following up on that.
| return minutes < 60 ? $"{minutes}m" : $"{minutes / 60}h {minutes % 60}m"; | ||
| } | ||
| 
               | 
          ||
| public static string GetTimeStringForScreenReaderFromMinutes(int minutes) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we fancy any unit tests for this?
| <table> | ||
| <tr class="header-row nhsuk-u-font-weight-bold"> | ||
| <th class="value header-row-content">Section</th> | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The NHS tables have specific roles for different table elements. Do we want them here?
https://service-manual.nhs.uk/design-system/components/table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe we need them since we are using the correct html elements, looking at the various bits of documentation on them e.g. https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/row_role.
e.g. I probably should have had them for the table structure with divs that I had before.
| 
               | 
          ||
| public static string GetTimeStringForScreenReaderFromMinutes(int minutes) | ||
| { | ||
| return minutes < 60 ? $"{minutes} minutes" : $"{minutes / 60} hours {minutes % 60} minutes"; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just double checking, I'm guessing we are fine with "1 hour 0 minutes" rather than going for just "1 hour" whenever we have a whole number of hours? Same sort of question would apply to the tag content of "1h 0m".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine where we have data down to the minute. Also the old site displays these as 1h 0m, and it's been to show and tell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, looks like Dan covered it
JIRA link
https://softwiretech.atlassian.net/browse/HEEDLS-871
Description
Fixed some bugs with sort and filter and some screen reader issues. Added category and topic tags.
Screenshots
Developer checks
(Leave tasks unticked if they haven't been appropriate for your ticket.)
I have: