-
Notifications
You must be signed in to change notification settings - Fork 4
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
Move temporary records functionality to crashes list #1654
base: nextjs
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for vision-zero-nextjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -203,7 +203,8 @@ const crashesListViewfilterCards: FilterGroup[] = [ | |||
{ | |||
id: "internal_filters", | |||
label: "Internal", | |||
groupOperator: "_or", | |||
// because one of the filters is enabled and inverted we need to join using the "_and" operator | |||
groupOperator: "_and", |
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.
👍 🙏
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.
@roseeichelmann thanks for making such quick work of this—it looks fantastic! One small thing: when you delete a temp crash, the CrashIsTemporaryBanner
is still routing to the old /create-crash-record
page. that can be updated to /crashes
.
@johnclary such a great catch, just fixed this! |
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.
🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢
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.
This fulfills the request on the issue perfectly -- 💯. Code looks great too -- thank you for this Rose!
My only suggestion is micro, and is related only to the label of the new filter and not the functionality. As is, though, this is plenty shippable. 🚢🚢🚢🚢
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.
This is totally outside the scope of this PR, but while I was reviewing it, a typo jumped out at me in this file. Would you mind adding a space to the comment /** Check local storage for initialsidebar state */
on line 42? No big if not!!
@@ -220,6 +221,20 @@ const crashesListViewfilterCards: FilterGroup[] = [ | |||
}, | |||
], | |||
}, | |||
{ | |||
id: "is_temp_record", | |||
label: "Temporary records", |
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.
What would you think about adding the word "Only" to the label here, so it reads "Only temporary records?" The idea being that the two internal filters have a very different interaction to the existing set.
"Include private drive crashes" adds to the existing set by allowing in more crashes, and "Temporary records" is unclear if it's going to limit to just temporary records or allow the temporary records to be added to the existing set being shown.
Because it ends up doing a different operation (limit as opposed to add-to) as the previous option, I think adding a verb to the label can help the user get the right expectation.
I feel like I've done a really poor job in explaining my rational - LMK if i can help explain any of this better.
Edit: I was reading more of this as I reviewed, and you pointed out how the one above is _and
and this one is still an _or
. They are the right operations, of course -- we just need to broadcast that nuance in the labels to the user.
Associated issues
Closes cityofaustin/atd-data-tech#20469
Testing
URL to test:
Local or netlify
Steps to test:
Ship list
main
branch