HEEDLS-416 Add details to admin cards #461
Conversation
...lLearningSolutions.Web/Views/TrackingSystem/Centre/Administrator/_SearchableAdminCard.cshtml
Outdated
Show resolved
Hide resolved
...lLearningSolutions.Web/Views/TrackingSystem/Centre/Administrator/_SearchableAdminCard.cshtml
Outdated
Show resolved
Hide resolved
...lLearningSolutions.Web/Views/TrackingSystem/Centre/Administrator/_SearchableAdminCard.cshtml
Outdated
Show resolved
Hide resolved
|
Re design, I think we should aim for the same button spacing as My account page (both desktop & mobile): I also wonder if there's a way to make the "this account is locked" text more visible.. There are no screenshots of the closed expanders, but could we try and make sure they have the NHS expander styling (I think they were using expander with buttons previously, which for some reason doesn't have the bottom border): |
...lLearningSolutions.Web/Views/TrackingSystem/Centre/Administrator/_SearchableAdminCard.cshtml
Outdated
Show resolved
Hide resolved
...lLearningSolutions.Web/Views/TrackingSystem/Centre/Administrator/_SearchableAdminCard.cshtml
Outdated
Show resolved
Hide resolved
| </div> | ||
| } | ||
| @foreach (var (displayText, filter) in Model.Tags) { | ||
| <div class="tag nhsuk-u-font-size-14" value="@filter"> |
There was a problem hiding this comment.
I wonder if we should leave adding these values until we determine that they're needed for JS to keep the MRs more concise and relevant to the ticket and to avoid forgetting to remove the code if the JS version approach ends up being different.. 🤔
There was a problem hiding this comment.
(unless you think it will be more work than worth it as I don't feel particularly strongly)
There was a problem hiding this comment.
I don't see any point removing them now I've already added them until I find they aren't needed. I think they'll be used given the approximate plan I have for the JS filtering
|
Change the layouts by taking out all the custom styles from the course cards that were messing them up. These are now in a common courses css file.
|
| </div> | ||
| } | ||
| @foreach (var (displayText, filter) in Model.Tags) { | ||
| <div class="tag nhsuk-u-font-size-14" value="@filter"> |
There was a problem hiding this comment.
Can you use value on divs? I thought it was only a property of HTML inputs..
https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement
VS
https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement
(but I might be missing something)
There was a problem hiding this comment.
Technically no but also kind of yes.
document.getElementById("id").value; won't work for this since it isn't a property of a div
But there isn't really anything to stop you putting random attributes on a div and using document.getElementById("id").getAttribute('value');
I guess I should probably just name it something else so it isn't confusing. e.g. filter-value
There was a problem hiding this comment.
Changed to data-filter-value
| @@ -5,6 +5,7 @@ | |||
|
|
|||
| <link rel="stylesheet" href="@Url.Content("~/css/shared/searchableElements.css")" asp-append-version="true"> | |||
| <link rel="stylesheet" href="@Url.Content("~/css/learningPortal/completed.css")" asp-append-version="true"> | |||
| <link rel="stylesheet" href="@Url.Content("~/css/learningPortal/courses.css")" asp-append-version="true"> | |||
There was a problem hiding this comment.
Could we not just import the courses.css in the completed.css with @import "courses.scss" to have the completed page css built in one stylesheet?
There was a problem hiding this comment.
Yes, and we probably could be importing the searchableElements in various places as well. Overall there's probably quite a lot of scss that is poorly set up in the project based on the amount of changes I was able to make when refactoring searchableElements.scss in 532.
|
Grey NHS tags
|
Populated the admin cards with more details. I found an extra dependency that will partially rely on whether the filtering work changes at all, marked this with a TODO. The structure of the html should hopefully be OK for the JS filtering work, but it might change slightly.
Checked the screen reader reads these as expected and ran all unit tests. Note that the pagination is still currently invalid html here, so it fails lighthouse. There are no other accessibility errors currently.