-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add code for filter and update dependencies #214
Conversation
Non filter related adjustments that I can across, just so I don't forget about having changed it when the PR gets finished up.
|
Added a fix for the query string issue and the pagination can persist the status filter. Here are some approaches I attempted along the way and was reminded of some issues with them: The URI methods ❌From looking into it, it seems there are methods to encode a string, turning special characters into something that can be sent in a query string. For example, Modify the asp-route element ❌At first, I attempted to switch the filter from a form to a route element and build a value to input into the Create my own string of statuses in the stimulus.js filter controller and send through an input in the filter form ✅After switching it back to a form and submitting my string of statuses to the route as a single string input delimited by commas and split into an array on the backend, I realized that the |
can still be split into filter methods for simple implementation
Pushed an implementation for our move away from js for the status filter. The filter now works with C# I feel that I can split some of these methods into a filter specific namespace for easier implementation in the future, but this may be a good approach to our issue. The OnGet for visit index requires default statuses in the query parameters, this is provided by page link buttons that go to the visit index. This is a means of setting a default and dynamic status value to the url. |
The OnGet visit index will now require a param of statuses or provide an error. This will help for development to prevent an incorrect page-route to the visit index. Without it, the visit index pagination will not work correctly and serves as making sure there is always a default statuses filter set.
If a page route is created to go to the visit index, this custom error will prevent developers from routing to the visit index without the correct route parameters provided for the filter. Without statuses parameters, the pagination links will not work correctly on first load of the visit index page. |
Filter Update
Implimentation
|
Filter structure has been adjusted
|
Reviewing... |
@mlan225 Feature branch behind main. Merging in main... no conflicts. |
@mlan225 Looking good overall and great iteration since the last review. There are some opportunities for simplifying and making FilterModel's constructor null safe. |
adjust model to expect string list constructor property for items
Address potential null errorsif selected items are not provided during construction, then it will default to an empty string list. My concern here would be if a developer were to not provide the parameter for selected items, then all values will always return selected as a result of defaulting to an empty list. Should I just make the selected items a required parameter? The query parameter will always return an array of count 0 if no selected items values are provided in the route BUT the query Parameters are still given during construction. Should the construction reach a compilation error if a selected items array of any length is not provided during construction or should I allow for a dev to not add it and get all items selected on every return until they provide the parameter? Adjust the constructor property type for items (enum of filter items)previously, the Enum.getValues method would return an Array type. I changed this to use getName to return the text of the enums and additionally, implemented the spread collection expression to simplify the value and return a list instead of an Array class type. With this change, you can now provide example: Model paramsCalling the constructor |
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.
@mlan225 We paired and made the code more readable and null safe. 👍
Resolves #210
Draft PR