Heedls 532 admins no js filtering #454

AlexJacksonDS · 2021-06-29T14:38:16Z

Added no javascript filtering of admin users in a (hopefully) fairly generic way that can be reused for other filtering. Some of the data structures are a bit weird here in a similar way to ones for the generic sorting.

The layout gets a bit odd at very small mobile layouts due to the size of the buttons, but it's probably OK. Checked that the screen reader reads it OK

DanBloxham-sw

A few comments to look at.

I'm also not seeing anything to indicate the filters are applied to a cookie - is that requirement being avoided or added in another subtask?

DigitalLearningSolutions.Data/DataServices/UserDataService.cs

DigitalLearningSolutions.Web/Views/TrackingSystem/Centre/Administrator/Index.cshtml

DigitalLearningSolutions.Web/Views/Shared/Components/CurrentFilters/Default.cshtml

DigitalLearningSolutions.Web/Styles/shared/searchableElements.scss

DigitalLearningSolutions.Web/ViewComponents/CurrentFiltersViewComponent.cs

DigitalLearningSolutions.Web/Views/TrackingSystem/Centre/Administrator/Index.cshtml

...ningSolutions.Web/Controllers/TrackingSystem/Centre/Administrator/AdministratorController.cs

AlexJacksonDS · 2021-06-30T14:57:18Z

I'm also not seeing anything to indicate the filters are applied to a cookie - is that requirement being avoided or added in another subtask?

I've created a separate subtask for this now. No idea how possible it will be anyway. Something to figure out once we have a working filter setup.

DanBloxham-sw

Looks good to me. Couple of comments I've left as unresolved as they are still open for discussion and can act as reminders for other tasks.

stellake · 2021-07-01T10:50:28Z

The mobile version does look a bit crowded.. Maybe worth trying to move the "Clear filters" button below the filter tags:

I also wonder if it's worth adding "Filters"/"Applied filters" text before the list of filters for extra clarity (especially useful in mobile view I think)

Having said that I'm actually surprised how good the desktop version looks! 👍

AlexJacksonDS · 2021-07-01T15:52:12Z

Desktop - added the labels and tidied up the spacing on the buttons so the text is vertically centred

Mobile - Clear filters button now collapses below the applied filters tags. Fixed the issue where the buttons fell onto the next row when the input was selected. (This was a problem with the search box on the live site too)

Also did some general css refactoring for the searchable elements classes. Removing some unused ones and pulling in some common settings. Hopefully 416 won't cause me a major merge headache.

stellake

Quick question - have we documented this anywhere so far?

stellake · 2021-07-05T07:54:37Z

DigitalLearningSolutions.Web/Views/TrackingSystem/Centre/Administrator/Index.cshtml

+      <input type="hidden" id="searchString" value="@Model.SearchString" />
+      <input type="hidden" id="filter-by" name="filterBy" value="@Model.FilterString" />
+      <input type="hidden" id="select-sort-by" value="Name" />
+      <input type="hidden" id="select-sort-direction" value="Ascending" />


Do you think we could use the asp-route-{value} as an alternative to all the hidden inputs in the markup?
https://docs.microsoft.com/en-us/aspnet/core/mvc/views/working-with-forms?view=aspnetcore-3.1#the-form-action-tag-helper

DigitalLearningSolutions.Web/Views/Shared/SearchablePage/_Pagination.cshtml

DigitalLearningSolutions.Web/Views/Shared/Components/CurrentFilters/Default.cshtml

...olutions.Web/ViewModels/TrackingSystem/Centre/Administrator/CentreAdministratorsViewModel.cs

stellake · 2021-07-05T08:23:47Z

DigitalLearningSolutions.Web/ViewModels/Common/FilterViewModel.cs

+
+        public string FilterName { get; set; }
+
+        public IEnumerable<(string DisplayText, string Filter)> FilterOptions { get; set; }


Maybe worth renaming the "Filter" to "Id" as a property "Filter" on "FilterOption" can get confusing

DigitalLearningSolutions.Web/ViewModels/Frameworks/AllFrameworksViewModel.cs

DigitalLearningSolutions.Web/ViewComponents/CurrentFiltersViewComponent.cs

DigitalLearningSolutions.Web/Helpers/FilteringHelper.cs

...b.Tests/ViewModels/TrackingSystem/Centre/Administrator/CentreAdministratorsViewModelTests.cs

stellake · 2021-07-05T08:57:59Z

DigitalLearningSolutions.Web.Tests/Helpers/FilteringHelperTests.cs

+        {
+            // Given
+            var inputItems = QueryableHelper.GetListOfSortableItems("a", 1, "a", 3, "b", 2);
+            var expectedItems = new[] { new SortableItem("a", 1), new SortableItem("a", 3) }.AsQueryable();


Given that we declare the sortableItems anyway, could we do something like (instead of using the helper):

var inputItems = new[] { item1, item2, item3 } var expectedItems = new[] { item1, item2 } ...

Would it also be worth refactoring
var item1 = new SortableItem("a", 1) (or even itemA1, itemA2, itemB1)
etc
so we don't need to declare these in lots of different places?

can do similarly in queryable extensions test

DigitalLearningSolutions.Web/Views/Shared/Components/CurrentFilters/Default.cshtml

stellake · 2021-07-06T12:54:14Z

DigitalLearningSolutions.Web/ViewModels/Common/FilterOptionViewModel.cs

+
+        public string DisplayText { get; set; }
+
+        public string Filter { get; set; }


I still think Filter could have a DisplayName, just like FilterOption. I wonder if "FilterId" or "FilterValue" would be more intuitive as a name 🤔

stellake · 2021-07-06T13:06:05Z

DigitalLearningSolutions.Web/ViewModels/Common/CurrentFiltersViewModel.cs

@@ -0,0 +1,17 @@
+namespace DigitalLearningSolutions.Web.ViewModels.Common


I wonder if we want to put all the search/sort/filter common classes under a new folder (maybe SearchSortFilter) as the Common folder is having quite a lot of classes now.

I wonder if it would also be worth putting the ViewComponent models into a folder at some point too

stellake · 2021-07-06T13:10:53Z

DigitalLearningSolutions.Web/ViewComponents/FilterableTagsViewComponent.cs

+    using DigitalLearningSolutions.Web.ViewModels.Common;
+    using Microsoft.AspNetCore.Mvc;
+
+    public class FilterableTagsViewComponent : ViewComponent


Could this be a partial instead if all we need is rendering a view with the model?

stellake · 2021-07-06T13:11:41Z

DigitalLearningSolutions.Web/ViewComponents/CurrentFiltersViewComponent.cs

+
+        private static bool FilterOptionsContainsFilter(
+            string currentFilter,
+            IEnumerable<FilterOptionViewModel> filter


Should this also be filterOptions? And should the "filterValue" be "filterOption"?

stellake · 2021-07-06T13:14:00Z

DigitalLearningSolutions.Web/Models/Enums/FilterStatus.cs

+        Default,
+        Warning,
+        Success,
+        Other //This will result in the default blue nhsuk-tag


Is there a reason we support "Other" at this stage?

"Having spoken to Carolyn and David, they agree that the first option ([blue,] grey, red, green) would be preferred. Thanks both."

Figured I might as well stick it in, but the name is probably bad

I thought that [blue] meant the colour of the listed filters, but I will clarify this with Kevin tomorrow morning.. Shouldn't block this though

Kevin confirmed that the blue NHS tag can go and we can use blue for applied filters

stellake · 2021-07-06T13:18:45Z

DigitalLearningSolutions.Web.Tests/Helpers/FilteringHelperTests.cs

+        {
+            // Given
+            var inputItems = QueryableHelper.GetListOfSortableItems("a", 1, "a", 3, "b", 2);
+            var expectedItems = new[] { new SortableItem("a", 1), new SortableItem("a", 3) }.AsQueryable();


Would it also be worth refactoring
var item1 = new SortableItem("a", 1) (or even itemA1, itemA2, itemB1)
etc
so we don't need to declare these in lots of different places?

can do similarly in queryable extensions test

stellake

Looks good - added a comment about the TagStatus blue not being required, but no need for re-review 👍

AlexJacksonDS added 8 commits June 25, 2021 10:59

HEEDLS-532 Initial no javascript filtering

d3eb901

HEEDLS-532 Add some styling to filters

90d9800

HEEDLS-532 Working filter and basic applied filter display

9c154af

HEEDLS-532 Add applied filter display and clear filters

9081857

HEEDLS-532 Populate categories filter from database

963f7b5

HEEDLS-532 Add unit tests for new code

7afc867

HEEDLS-532 Fix layout for mobile

69618f6

HEEDLS-532 Improve screen reader reading

d7071af

DanBloxham-sw reviewed Jun 30, 2021

View reviewed changes

AlexJacksonDS added 4 commits June 30, 2021 14:31

HEEDLS-532 Break up large linq statement

3d40a9b

Merge branch 'master' into HEEDLS-532-admins-no-js-filtering

270b15c

HEEDLS-532 Fix naming of IConfiguration

32d984b

HEEDLS-416 Increase spacing after number of admins view component

a9baaf7

DanBloxham-sw approved these changes Jul 1, 2021

View reviewed changes

HEEDLS-532 Change filters IEnumerable to a model

1c20c0e

AlexJacksonDS added 2 commits July 1, 2021 16:40

HEEDLS-532 Improve mobile layout and css refactoring

99c8fe3

HEEDLS-532 Fix broken scss include

34079fc

Merge branch 'master' into HEEDLS-532-admins-no-js-filtering

e120cd7

stellake reviewed Jul 5, 2021

View reviewed changes

AlexJacksonDS added 6 commits July 5, 2021 10:40

HEEDLS-532 Add coloured NHS tags

97bb3fe

HEEDLS-532 Review markups

c819b3d

HEEDLS-532 Revert changes to pagination covered by another ticket

d8ef99d

HEEDLS-532 Check whether user is a CMS manager properly

6d60283

HEEDLS-532 Add an extra Other filter status for the default blue tag

510a970

HEEDLS-532 Fix unit test

284acc0

stellake reviewed Jul 6, 2021

View reviewed changes

HEEDLS-532 Move SearchablePage view models into folder

501946f

AlexJacksonDS added 6 commits July 6, 2021 15:36

HEEDLS-532 Move view component models into folder

1c2b179

HEEDLS-532 Rename filter value

03fa1f1

HEEDLS-532 Refactor filter/queryable unit tests

ff80603

Merge branch 'master' into HEEDLS-532-admins-no-js-filtering

56b351f

HEEDLS-532 Refactor css into multiple files

270d50e

HEEDLS-532 Remove Other FilterStatus

ce28323

stellake approved these changes Jul 7, 2021

View reviewed changes

AlexJacksonDS merged commit 2732d97 into master Jul 7, 2021

AlexJacksonDS deleted the HEEDLS-532-admins-no-js-filtering branch July 7, 2021 09:24


		public string FilterName { get; set; }

		public IEnumerable<(string DisplayText, string Filter)> FilterOptions { get; set; }


		public string DisplayText { get; set; }

		public string Filter { get; set; }

		@@ -0,0 +1,17 @@
		namespace DigitalLearningSolutions.Web.ViewModels.Common

Heedls 532 admins no js filtering #454

Heedls 532 admins no js filtering #454

Uh oh!

Conversation

AlexJacksonDS commented Jun 29, 2021

Uh oh!

DanBloxham-sw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexJacksonDS commented Jun 30, 2021

Uh oh!

DanBloxham-sw left a comment

Choose a reason for hiding this comment

Uh oh!

stellake commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexJacksonDS commented Jul 1, 2021

Uh oh!

stellake left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stellake left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stellake commented Jul 1, 2021 •

edited

Loading