feat: Filters support choosing any flagset category #3797
Conversation
|
Removed vultr server and associated DNS entries |
| name="category" | ||
| value={formik.values.category} | ||
| onChange={formik.handleChange} | ||
| > |
There was a problem hiding this comment.
My current thinking is that we can merge this to main with a simple disabled prop added here?
Then I can rebase into #3764, correctly adjust filter automations, and un-disable? Does this sound solid, anything I might be forgetting?
Please test flows on this pizza with existing filter components to confirm they work correctly before any type of node "Update" !
| id?: string; | ||
| handleSubmit?: (d: any, children: any) => void; | ||
| node?: any; | ||
| } |
There was a problem hiding this comment.
This was unused! Removed to avoid confusion, we're simply reading props defined at the top of Editor.tsx.
| ); | ||
|
|
||
| fireEvent.change(screen.getByTestId("flagset-category-select"), { | ||
| target: { value: "Community infrastructure levy" }, |
There was a problem hiding this comment.
question: why would we use fireEvent here rather than userEvent?
Does it make selecting from a <select> easier?
(rather than going through user.click, find the options, user.click again...)
There was a problem hiding this comment.
I'm using fireEvent throughout this file because the modal submit buttons like "Create filter" / "Update filter" aren't easily accessible to user.click() - therefore fireEvent.submit() is handy to have (this is commonly used throughout other "Editor" test files as well).
I think the select interaction specifically could be migrated to userEvent going forward - which would mean we'd be testing the interaction more realistically to how the user would experience it in the browser.
| value={formik.values.category} | ||
| onChange={formik.handleChange} | ||
| > | ||
| {Array.from(categories).map((category) => ( |
There was a problem hiding this comment.
First time seeing this, its so slick with the new Set() above!
There was a problem hiding this comment.
I ran through a prior approval check and from my understanding I ended up where I expected with the existing filters in place. Although it would be nice to have a session on this, I'll put something in Slack
Only one comment on the Editor Modal and one on using fireEvent in the tests. Both are nits and don't require action, more feedback on my expectations.
Rebasing on to the other PR mentioned sounded right and I couldn't think of anything missed here
There was a problem hiding this comment.
nit: I'd expect the Editor model to be similar to the Result component, where I can see the filter options before updating. I am an uneducated user, so it is feasible that this is redundant for people who know what to expect from the filters.
There was a problem hiding this comment.
Filter options are not editable, they are hard-coded only still - therefore in my opinion there's no reason to display them in the modal here (they'd have to be disabled fields?).
In the future, when we may have customisable filter options (aka flags and flagsets), then I imagine it will make sense to expose them directly here rather select from a pre-existing/pre-configured category ! But that's out of scope of these changes.
Changes:
<select />styling as External Portal and Result modals, let's revisit styles latercollectedFlagsstore method here, which means that all non-"Planning permission" filters won't actually "work" yet - in that they aren't correctly reading collected flags and still exclusively route down the "No flag result" branch