Crash details: make change log collapsible #1655

roseeichelmann · 2025-01-10T20:17:06Z

Associated issues

Closes cityofaustin/atd-data-tech#19968

Testing

URL to test:
Local or netlify

Steps to test:

Go to a crash details page, scroll down to the change log. It should be collapsed on default first load.
Now expand it and refresh the page. The new state should have saved in local storage so on refresh it should still be expand it.
Collapse it and refresh again, making sure your state is saved. Go to other crashes and make sure the saved state is persisting.

Ship list

Check migrations for any conflicts with latest migrations in main branch
Confirm Hasura role permissions for necessary access
Code reviewed
Product manager approved

netlify · 2025-01-10T20:19:02Z

✅ Deploy Preview for vision-zero-nextjs ready!

Name	Link
🔨 Latest commit	`9ba19e6`
🔍 Latest deploy log	https://app.netlify.com/sites/vision-zero-nextjs/deploys/678596800f9cf80008e03346
😎 Deploy Preview	https://deploy-preview-1655--vision-zero-nextjs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

roseeichelmann · 2025-01-10T20:27:12Z

app/components/ChangeLog.tsx

+      defaultActiveKey={expandedItem}
+      // on accordion click save new expanded item state to local storage
+      onSelect={(e) => {
+        const localStorageValue = e !== null ? "0" : JSON.stringify(null); // local storage value must be a string type


i was getting a typescript linting error when just setting to null "Type 'null' is not assignable to type 'string'." and this is just never something i would have been super aware before programming in a strongly typed language. its weird but interesting having to take into account the parameter typing of external methods that we dont control, like setItem,

i'm glad you shared about this in dev sync because it is a TIL for me too! Purely in opinion zone here but I think it would be okay to use the String constructor to make null into a string to work around this too. 🤷

I do think the e could be change to eventKey after I found myself wondering how these Bootstrap accordions work. The docs show that the first parameter in the callback is a string and the second is an event object which I expected this e to be when I read the code.

I think it would be okay to use the String constructor to make null into a string to work around this to

i like this suggestion a lot. my inclination would be to restrict expandedItem to being a string only, and deal with conversion to a string with the String constructor. And that way we can always store whatever String(e) is into local storage without caring about it's value beyond that.

also, for what it's worth we don't have to use "0" as the key value. we could use something that's more obviously a string, like "recordHistory". and it might to be good to stash that value as const var 🤷

One other detail @roseeichelmann—I think that instead of the defaultActiveKey prop, the activeKey prop is a better choice to use instead. i see this kind of like using the value prop instead of defaultValue prop on a controlled form input.

these are all great comments, thanks yall! just pushed these changes

mddilley

Looks great and I'm happy to see this feature carrying over into the new app. I left one suggestion for the code, but this works just as expected! 💯

mddilley · 2025-01-10T23:39:27Z

app/components/ChangeLog.tsx

+      defaultActiveKey={expandedItem}
+      // on accordion click save new expanded item state to local storage
+      onSelect={(e) => {
+        const localStorageValue = e !== null ? "0" : JSON.stringify(null); // local storage value must be a string type


i'm glad you shared about this in dev sync because it is a TIL for me too! Purely in opinion zone here but I think it would be okay to use the String constructor to make null into a string to work around this too. 🤷

I do think the e could be change to eventKey after I found myself wondering how these Bootstrap accordions work. The docs show that the first parameter in the callback is a string and the second is an event object which I expected this e to be when I read the code.

johnclary

Rose—this is looking really good. I requested to changes to tighten up some typing and naming.

I also want to remark that I'd like to nix the blue background on the accordion header when it's expanded. but i am happy to stash that in a follow-up issue. as part of that follow-up work it would be nice toto make the header look the same as the bootstrap Card header for consistency with the other cards on the page, i think?

johnclary · 2025-01-13T15:10:25Z

app/components/ChangeLog.tsx

 import Table from "react-bootstrap/Table";
+import Accordion from "react-bootstrap/Accordion";
 import { formatDateTime } from "@/utils/formatters";
 import ChangeLogDetails from "./ChangeLogDetails";


could convert to an absolute import 😇

johnclary · 2025-01-13T15:22:08Z

app/components/ChangeLog.tsx

+      defaultActiveKey={expandedItem}
+      // on accordion click save new expanded item state to local storage
+      onSelect={(e) => {
+        const localStorageValue = e !== null ? "0" : JSON.stringify(null); // local storage value must be a string type


I think it would be okay to use the String constructor to make null into a string to work around this to

i like this suggestion a lot. my inclination would be to restrict expandedItem to being a string only, and deal with conversion to a string with the String constructor. And that way we can always store whatever String(e) is into local storage without caring about it's value beyond that.

also, for what it's worth we don't have to use "0" as the key value. we could use something that's more obviously a string, like "recordHistory". and it might to be good to stash that value as const var 🤷

One other detail @roseeichelmann—I think that instead of the defaultActiveKey prop, the activeKey prop is a better choice to use instead. i see this kind of like using the value prop instead of defaultValue prop on a controlled form input.

johnclary · 2025-01-13T15:25:04Z

app/components/ChangeLog.tsx

@@ -9,6 +9,9 @@ import {
  ChangeLogEntryEnriched,
 } from "@/types/changeLog";

+// used to track the accordion expanded state of the change log
+const localStorageKey = "expandedItem";


would you mind making this a little more descriptive and specific instead of "expandedItem", like "crashHistoryCardExpandedItem"?

btw, i created cityofaustin/atd-data-tech#20575 to add in support for version local storage across the app 👍

frankhereford

This looks super Rose. Really well done. I think the comments from other reviewers all make sense and get support from me, and I added one little thing concerning a color choice, but that's hardly enough to stop this from shipping. Send it out to sea!

🚢🚢🚢🚢🚢

frankhereford · 2025-01-13T16:11:05Z

app/components/ChangeLog.tsx

+      }}
+    >
+      <Accordion.Item eventKey="0">
+        <Accordion.Header>Record history</Accordion.Header>


This header gets a baby blue color when it's expanded, and it's the only header row in on the page to get that color. Can it be rgba(33, 37, 41, 0.03) like the rest of of the card headers?

agreed—and i created cityofaustin/atd-data-tech#20606 to follow-up on this 🙏

johnclary

Looks great + works great!

johnclary · 2025-01-14T15:38:03Z

app/components/ChangeLog.tsx

-        const localStorageValue = e !== null ? "0" : JSON.stringify(null); // local storage value must be a string type
+      onSelect={(eventKey) => {
+        const localStorageValue =
+          eventKey !== null ? recordHistoryItemName : String(null); // local storage value must be a string type


nbd, but you can do away with the ternary and use String(eventKey) as the localStorageValue since it will always be null or the recordHistoryItemName.

+1 - i think that removing this ternary would make more readable 💯

johnclary · 2025-01-14T15:39:49Z

app/components/ChangeLog.tsx

+      }}
+    >
+      <Accordion.Item eventKey="0">
+        <Accordion.Header>Record history</Accordion.Header>


agreed—and i created cityofaustin/atd-data-tech#20606 to follow-up on this 🙏

mddilley

Thanks for making these adjustments! I retested, and I can still save my preferred layout. 🙌 🚀 🚢

roseeichelmann added 2 commits January 10, 2025 14:05

add accordion component and set local storage on accordion expand

5dc3959

typo fixes

d8e6493

roseeichelmann commented Jan 10, 2025

View reviewed changes

roseeichelmann changed the title ~~Crash details: make change log collapsable~~ Crash details: make change log collapsible Jan 10, 2025

roseeichelmann requested review from johnclary, frankhereford, mateoclarke, chiaberry, tillyw and mddilley January 10, 2025 21:36

mddilley reviewed Jan 10, 2025

View reviewed changes

johnclary requested changes Jan 13, 2025

View reviewed changes

frankhereford approved these changes Jan 13, 2025

View reviewed changes

clear up some var names, absolute import paths, use String constructor

9ba19e6

roseeichelmann requested review from johnclary and mddilley January 13, 2025 22:43

johnclary mentioned this pull request Jan 14, 2025

Refine crash history card style cityofaustin/atd-data-tech#20606

Open

johnclary approved these changes Jan 14, 2025

View reviewed changes

mddilley approved these changes Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Crash details: make change log collapsible #1655

Crash details: make change log collapsible #1655

roseeichelmann commented Jan 10, 2025

netlify bot commented Jan 10, 2025 •

edited

Loading

roseeichelmann Jan 10, 2025

mddilley Jan 10, 2025 •

edited

Loading

johnclary Jan 13, 2025 •

edited

Loading

roseeichelmann Jan 13, 2025

mddilley left a comment

mddilley Jan 10, 2025 •

edited

Loading

johnclary left a comment •

edited

Loading

johnclary Jan 13, 2025

johnclary Jan 13, 2025 •

edited

Loading

johnclary Jan 13, 2025

johnclary Jan 13, 2025

frankhereford left a comment

frankhereford Jan 13, 2025

johnclary Jan 14, 2025

johnclary left a comment

johnclary Jan 14, 2025

mddilley Jan 14, 2025

johnclary Jan 14, 2025

mddilley left a comment

Crash details: make change log collapsible #1655

Are you sure you want to change the base?

Crash details: make change log collapsible #1655

Conversation

roseeichelmann commented Jan 10, 2025

Associated issues

Testing

Ship list

netlify bot commented Jan 10, 2025 • edited Loading

✅ Deploy Preview for vision-zero-nextjs ready!

Choose a reason for hiding this comment

mddilley Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

johnclary Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mddilley left a comment

Choose a reason for hiding this comment

mddilley Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

johnclary left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnclary Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankhereford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnclary left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mddilley left a comment

Choose a reason for hiding this comment

netlify bot commented Jan 10, 2025 •

edited

Loading

mddilley Jan 10, 2025 •

edited

Loading

johnclary Jan 13, 2025 •

edited

Loading

mddilley Jan 10, 2025 •

edited

Loading

johnclary left a comment •

edited

Loading

johnclary Jan 13, 2025 •

edited

Loading