Skip to content
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 zoom & rotate functionality to crash diagrams #1635

Merged
merged 53 commits into from
Jan 22, 2025

Conversation

frankhereford
Copy link
Member

@frankhereford frankhereford commented Dec 20, 2024

Associated issues

This PR hopes to close cityofaustin/atd-data-tech#19967.

Ask forgiveness, not permission

Here's the guiding image:

Image 394877044 924x1134

Building out from the image mentally -- I took some liberties, but have done my best to stay true to the new nextjs visual theme and aesthetic. I'm very 👍 on the VZE style and hope we perfect it.

But to be specific, here's some of the things that are big enough changes from the image to point out. I'm really leaning into only including the elements indicated by the arrows in the guiding image.

  • Recolor the button controls in the primary color like the 'Edit' button
  • Increase the size of the iconography in the buttons.
  • Replace the "do again" symbol with the "undo" symbol, but I did have to grab an icon from another font, not bootstrap. I think it matches really well.
  • Drop the redundant "Reset" button from near the rotation control. I wired up the top-right "undo" button to reset both zoom and rotation.
  • Drop the instructive label on the rotation handle. Users are repeat users for us, and any curious person will teach themselves what this harmless handle does real quick, and we can avoid the clutter.

Testing

  • Grab the deploy preview link from below in the 🤖 comment and check out some crash diagrams.
  • Do all the things, see if you can break them.
  • Consider the stylistic choices I have made.
    • Does this belong on this page?
    • Are you confused about what anything means...
      • ... after you play with it for a few seconds?
  • Visit a temp crash page to see what error messages look like. I found these crashes by loading local dev with production data.
    • Look good?
    • Are you cool that you can slide them around a little like an image, but they snap back?
      • I kinda dig how it's a harmless fidget toy in a way, and it's kinda hard to disable that functionality, I think. I do hide the image manipulation buttons though.

PS: DX experience

I absolutely felt a pang of joy when I could click a link in the CI PR comment and get a build log from Netlify that clearly indicated what the build problem was, like only typescript will do. It was so easy to find and fix where I went sideways. Typescript rocks! 🐐


Ship list

  • Code reviewed
  • Product manager approved

@frankhereford frankhereford self-assigned this Dec 20, 2024
Copy link

netlify bot commented Dec 20, 2024

Deploy Preview for vision-zero-nextjs ready!

Name Link
🔨 Latest commit c976f68
🔍 Latest deploy log https://app.netlify.com/sites/vision-zero-nextjs/deploys/67914a19f5f5b000088bd340
😎 Deploy Preview https://deploy-preview-1635--vision-zero-nextjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link
Contributor

@roseeichelmann roseeichelmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great Frank! Love seeing everyone's typescript and i learn a little from each PR i review ✨
I like your UX choices to cut down on the number of reset buttons we have, i think having just one that resets the image completely is perfect.

I noticed that when I click on crashes with an error banner, the zoom/reset tools and the words "crash diagram" will be on the screen for awhile before the banner takes over. Sometimes its for long enough that i can even move around and zoom on the words and then some buggy behavior starts happening. If you could prevent those tools and those words from popping up before the banner shows that would be awesome! I'm also getting the pesky horizontal scroll on the temporary banner when im at 80% page zoom.
Screencast from 01-06-2025 01:10:09 PM.webm

On the other banner, if you could center it horizontally that would scratch a great itch for me too heh
image

Copy link
Contributor

@roseeichelmann roseeichelmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great Frank! Love seeing everyone's typescript and i learn a little from each PR i review ✨
I like your UX choices to cut down on the number of reset buttons we have, i think having just one that resets the image completely is perfect.

I noticed that when I click on crashes with an error banner, the zoom/reset tools and the words "crash diagram" will be on the screen for awhile before the banner takes over. Sometimes its for long enough that i can even move around and zoom on the words and then some buggy behavior starts happening. If you could prevent those tools and those words from popping up before the banner shows that would be awesome! I'm also getting the pesky horizontal scroll on the temporary banner when im at 80% page zoom.
Screencast from 01-06-2025 01:10:09 PM.webm

On the other banner, if you could center it horizontally that would scratch a great itch for me too heh
image

@roseeichelmann
Copy link
Contributor

also i noticed you added helper text to the zoom/reset buttons but i think those could be left as just symbols and if anything its the slide bar that could use some labeling, or those icons like @mateoclarke suggested look great!

@johnclary
Copy link
Member

johnclary commented Jan 7, 2025

Thanks everyone for the design feedback. @frankhereford here's a summary of where i'd like to go with this feature in scope of this PR.

  1. Replace the CSS sizing with flexbox, like I mentioned in this comment. I'm very happy to pair on this—I know these can be a bit tricky
  2. Let's go ahead and pull the Alert banners out of the part of the transform component so they don't wiggle around. So conditionally render either the transform component or the Alert banners.
  3. Remove labels from zoom buttons (sorry for my miscommunication on that!) and add a rotate icon to the card footer. Here's a little design i drew up for this:
Screenshot 2025-01-07 at 1 01 14 PM

And here are a few bits of useful JSX i used for the design.

First, the card footer, which has the FaRotate icon in primary next to the RotateControl

        <Card.Footer className="text-center">
          <div className="d-flex align-items-center">
            <div className="me-3 text-primary fs-5">
                <FaRotate />
            </div>
            <RotateControls rotation={rotation} setRotation={setRotation} />
          </div>
        </Card.Footer>

And a small change to the RotateControl itself—adding w-100 to the outer div and removing the style entirely from the Form.Range component:

    <div className="mt-2 w-100">
      <Form.Range
        min="-180"
        max="180"
        value={rotation}
        id="formControlRange"
        onChange={rotate}
        title="Rotate Diagram"
      />
    </div>

Thanks for bearing with all the feedback on this one!

@frankhereford
Copy link
Member Author

@johnclary I think this is ready to be handed back to you. I believe I have incorporated the changes you requested, and I am super grateful for the helpful examples. I have used them verbatim where I was able and made only minimal changes where I was not. That was very kind of you to put that much effort into the comment like you did. 🙏

@roseeichelmann I've also asked for your review again because I believe the input you were asking had been incorporated, where applicable, into John's revised request. If you think I have overlooked something you want to see done, please let me know, and I'll be all over it. Thanks so much for all your thoughtful feedback.

@chiaberry
Copy link
Member

chiaberry commented Jan 14, 2025

I know @johnclary requested the rotate icon to be in primary, but that made me think it was a button and I tried to click it, thinking it would reset or rotate the image by 90 degrees.

I think being able to zoom out to make the image smaller and less detailed than what it shows on-load an anti-feature.

I have noticed that if you rotate the image, the edges fall out of the viewport, which could be why theres the option to zoom out in existing production. Of course, I don't know if VZE users would even miss this change in functionality.

@johnclary I looked in the readme todo's and github issues for the download cr3 button, I can't remember if you said it was left off the ACC designs intentionally or not, that was so last year 😬. Just don't want it to fall through the cracks

Copy link
Member

@johnclary johnclary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frankhereford this is looking great and is so so close. for some reason the diagram image is still overflowing the transform component on my display size, causing it to be partly obscured. i spent a few minutes trying to diagnose this and couldn't sort it out.

i am happy to open a follow-up for this if it feels like you want a break from this. The goal being to have the full diagram image displayed on init without having to zoom out.

Here's a screenshot showing the image pushing under the rotate control for some reason 🫠

Screenshot 2025-01-15 at 2 10 35 PM

app/components/CrashDiagramCard.tsx Outdated Show resolved Hide resolved
app/components/CrashDiagramCard.tsx Outdated Show resolved Hide resolved
app/components/CrashDiagramCard.tsx Outdated Show resolved Hide resolved
@frankhereford
Copy link
Member Author

@johnclary and everyone -- thank you all for your feedback on this. I believe I have incorporated all the requests, and I have been liberal in my use of the Resolve Conversation feature here in GitHub to try to pare down this PR for approachability, but I do not believe I have used it without addressing feedback.

The most notable change is now we have a zoomToImage() function which will zoom to the image, based on the dimensions of the component and the image itself. I'm calling this onLoad of the Image component -- thank you NextJS for that handy callback opportunity. This same function is used in the "Reset" button now as well.

I can't wait to hear what you think -- and thank you again for the chance work on my JS and contribute to the app.

Copy link
Member

@johnclary johnclary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

boom! you did it! thank you, Frank!!!

i requested the tiniest change so as to nix the onload animation. but i'm approving it now 🚢 🚢 🚢 🚢 🚢

app/components/CrashDiagramCard.tsx Outdated Show resolved Hide resolved
@frankhereford frankhereford merged commit b43504a into nextjs Jan 22, 2025
4 checks passed
@frankhereford frankhereford deleted the frank/diagram-gizmos branch January 22, 2025 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants