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

Update our left-hand nav icons #9393

Merged
merged 11 commits into from Jun 29, 2023
Merged

Update our left-hand nav icons #9393

merged 11 commits into from Jun 29, 2023

Conversation

jonbell-lot23
Copy link
Contributor

@jonbell-lot23 jonbell-lot23 commented Jun 28, 2023

Summary

This update standardizes our icons to feel more like a family, plus makes some aesthetic improvements. Before and after:
image

@vercel
Copy link

vercel bot commented Jun 28, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
devtools ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 29, 2023 7:01am

@replay-io
Copy link

replay-io bot commented Jun 28, 2023

E2E Tests

74 replays were recorded for 3e730d2.

image 0 Failed
image 74 Passed

View test run on Replay ↗︎

Snapshot Tests

100 replays were recorded for 3e730d2.

image 0 Failed
image 100 Passed

View test run on Replay ↗︎

@jonbell-lot23 jonbell-lot23 marked this pull request as ready for review June 28, 2023 07:00
@jonbell-lot23 jonbell-lot23 changed the title [DRAFT] Update our left-hand nav icons Jun 28, 2023
Copy link
Collaborator

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

Nit: The first icon feels too close to the top. (Seems like the padding/gutter around the icon, and between each icon, should be even?)

Screen Shot 2023-06-28 at 12 08 12 PM

src/ui/components/shared/IconWithTooltip.tsx Outdated Show resolved Hide resolved
src/ui/components/shared/IconWithTooltip.tsx Outdated Show resolved Hide resolved
src/ui/components/Toolbar.tsx Show resolved Hide resolved
@bvaughn
Copy link
Collaborator

bvaughn commented Jun 28, 2023

It's late and I need to get to bed. I'll provide a more detailed accounting tomorrow.

FWIW the “Draft” PR status is good to use in this situation. It lets the reviewer know to hold off until the PR is ready 😄

@jonbell-lot23
Copy link
Contributor Author

jonbell-lot23 commented Jun 28, 2023

Nit: The first icon feels too close to the top. (Seems like the padding/gutter around the icon, and between each icon, should be even?)

image

Yup, there's even spacing between each icon, then the gap at the top is exactly half. This is appropriate (but open to differences of opinion of course) because it's the top of a grouping of icons. The composition needs to work against other items (the title and back arrow), so keeping the exact same spacing there could work, but isn't a given. In our case, it makes the icons look too low. (in my opinion) The other thing I tried was zero top margin so it aligns with the top of the panel, which I think can work too.

@bvaughn
Copy link
Collaborator

bvaughn commented Jun 28, 2023

Guess we just have different subjective opinions in that regard. I think the half padding looks broken 🤷🏼 Thanks for explaining your reasoning though

@jonbell-lot23
Copy link
Contributor Author

Guess we just have different subjective opinions in that regard. I think the half padding looks broken 🤷🏼 Thanks for explaining your reasoning though

I definitely prefer when we agree, but yeah, some things are just subjective. VS Code does a similar thing:
image

What do you think about removing the padding entirely so it aligns evenly with the top of the panel? I think that could work.

Copy link
Collaborator

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

I definitely prefer when we agree, but yeah, some things are just subjective.

Me too! 😄 Doesn't always happen though, which is fine.

Seeing it in context, the alignment looks better bc there's space in the row above:
Screen Shot 2023-06-28 at 2 43 16 PM

That being said, I think there's something funky going on with the icon's own internal vertical alignment:
Screen Shot 2023-06-28 at 2 46 51 PM

Compare it to one of the other icons, which is properly centered (vertically and horizontally):
Screen Shot 2023-06-28 at 2 46 59 PM

Where did the SVG paths come from?

@jonbell-lot23
Copy link
Contributor Author

Where did the SVG paths come from?

These are from Apple's symbols library. In the case of the info, I modified their icon because it started like this:
image

And here's how the dimensions look on the artboard:
image

@bvaughn
Copy link
Collaborator

bvaughn commented Jun 28, 2023

Looks like something went wrong during the export/import step then. Looking at the preview branch for this PR–

If I copy that icon out:

<svg width="24" height="24" viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg" class="h-full w-full" fill="currentColor"><path d="M12.1879 22.8578C10.8187 22.8578 9.5292 22.5952 8.3195 22.0701C7.11645 21.5517 6.05298 20.8339 5.12908 19.9167C4.21184 18.9928 3.49068 17.9294 2.96559 16.7262C2.44715 15.5165 2.18793 14.2271 2.18793 12.8579C2.18793 11.4886 2.44715 10.2025 2.96559 8.99945C3.49068 7.78974 4.21184 6.72627 5.12908 5.80903C6.04634 4.88514 7.10648 4.16397 8.30954 3.64553C9.51923 3.12044 10.8087 2.85789 12.1779 2.85789C13.5472 2.85789 14.8366 3.12044 16.0463 3.64553C17.256 4.16397 18.3195 4.88514 19.2368 5.80903C20.154 6.72627 20.8751 7.78974 21.4002 8.99945C21.9253 10.2025 22.1879 11.4886 22.1879 12.8579C22.1879 14.2271 21.9253 15.5165 21.4002 16.7262C20.8751 17.9294 20.154 18.9928 19.2368 19.9167C18.3195 20.8339 17.256 21.5517 16.0463 22.0701C14.8433 22.5952 13.5572 22.8578 12.1879 22.8578ZM12.1879 21.5717C13.391 21.5717 14.5177 21.3457 15.5677 20.8937C16.6246 20.4418 17.5519 19.8169 18.3494 19.0194C19.1537 18.2218 19.7784 17.2978 20.2238 16.2476C20.6758 15.1909 20.9018 14.0609 20.9018 12.8579C20.9018 11.6548 20.6758 10.5282 20.2238 9.478C19.7718 8.42118 19.147 7.49397 18.3494 6.69636C17.5519 5.89211 16.6246 5.26732 15.5677 4.82199C14.5177 4.37002 13.3877 4.14403 12.1779 4.14403C10.9749 4.14403 9.84492 4.37002 8.7881 4.82199C7.73792 5.26732 6.81403 5.89211 6.01642 6.69636C5.22547 7.49397 4.604 8.42118 4.15203 9.478C3.7067 10.5282 3.48403 11.6548 3.48403 12.8579C3.48403 14.0609 3.7067 15.1909 4.15203 16.2476C4.604 17.2978 5.22878 18.2218 6.02639 19.0194C6.82399 19.8169 7.74789 20.4418 8.79807 20.8937C9.84824 21.3457 10.9782 21.5717 12.1879 21.5717Z"></path><path d="M13.1374 11.8119C13.1374 11.3183 12.7373 10.9182 12.2437 10.9182C11.7502 10.9182 11.3501 11.3183 11.3501 11.8119V18.0964C11.3501 18.5899 11.7502 18.99 12.2437 18.99C12.7373 18.99 13.1374 18.5899 13.1374 18.0964V11.8119Z"></path><path d="M11.4692 8.75378C11.6819 8.95983 11.9377 9.06286 12.2369 9.06286C12.5426 9.06286 12.7985 8.95983 13.0046 8.75378C13.2173 8.54109 13.3236 8.2852 13.3236 7.9861C13.3236 7.68035 13.2173 7.42113 13.0046 7.20843C12.7985 6.99574 12.5426 6.88939 12.2369 6.88939C11.9377 6.88939 11.6819 6.99574 11.4692 7.20843C11.2565 7.42113 11.1501 7.68035 11.1501 7.9861C11.1501 8.2852 11.2565 8.54109 11.4692 8.75378Z"></path></svg>

And blow it up larger, it looks like this:
Screen Shot 2023-06-28 at 3 40 39 PM

@bvaughn
Copy link
Collaborator

bvaughn commented Jun 28, 2023

I think all of the other icons look okay, except this one is also misaligned:

<svg width="24" height="24" viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg" class="h-full w-full" fill="currentColor"><path d="M3.3327 11.2971C3.3327 10.3303 3.51434 9.42505 3.87762 8.5813C4.24091 7.73169 4.74481 6.98462 5.38934 6.34009C6.03387 5.69556 6.77802 5.19165 7.62177 4.82837C8.47137 4.46509 9.37958 4.28345 10.3464 4.28345C11.3132 4.28345 12.2184 4.46509 13.0622 4.82837C13.9118 5.19165 14.6589 5.69556 15.3034 6.34009C15.9479 6.98462 16.4518 7.73169 16.8151 8.5813C17.1784 9.42505 17.36 10.3303 17.36 11.2971C17.36 12.0999 17.2311 12.8616 16.9733 13.5823C16.7214 14.303 16.3698 14.9563 15.9186 15.5422L20.2165 19.8665C20.3102 19.9602 20.3806 20.0686 20.4274 20.1917C20.4802 20.3147 20.5065 20.4465 20.5065 20.5872C20.5065 20.7805 20.4626 20.9563 20.3747 21.1145C20.2927 21.2727 20.1755 21.3958 20.0231 21.4836C19.8708 21.5774 19.695 21.6243 19.4958 21.6243C19.3552 21.6243 19.2204 21.5979 19.0915 21.5452C18.9684 21.4983 18.8542 21.425 18.7487 21.3254L14.4245 16.9924C13.8503 17.4026 13.2175 17.7249 12.5261 17.9592C11.8347 18.1936 11.1081 18.3108 10.3464 18.3108C9.37958 18.3108 8.47137 18.1292 7.62177 17.7659C6.77802 17.4026 6.03387 16.8987 5.38934 16.2542C4.74481 15.6096 4.24091 14.8655 3.87762 14.0217C3.51434 13.1721 3.3327 12.2639 3.3327 11.2971ZM4.83563 11.2971C4.83563 12.0588 4.97626 12.7737 5.25751 13.4417C5.54462 14.1038 5.94012 14.6868 6.44403 15.1907C6.9538 15.6946 7.53973 16.0901 8.20184 16.3772C8.86981 16.6643 9.58466 16.8079 10.3464 16.8079C11.1081 16.8079 11.82 16.6643 12.4821 16.3772C13.1501 16.0901 13.736 15.6946 14.2399 15.1907C14.7438 14.6868 15.1393 14.1038 15.4265 13.4417C15.7136 12.7737 15.8571 12.0588 15.8571 11.2971C15.8571 10.5354 15.7136 9.82349 15.4265 9.16138C15.1393 8.49341 14.7438 7.90747 14.2399 7.40356C13.736 6.8938 13.1501 6.49829 12.4821 6.21704C11.82 5.92993 11.1081 5.78638 10.3464 5.78638C9.58466 5.78638 8.86981 5.92993 8.20184 6.21704C7.53973 6.49829 6.9538 6.8938 6.44403 7.40356C5.94012 7.90747 5.54462 8.49341 5.25751 9.16138C4.97626 9.82349 4.83563 10.5354 4.83563 11.2971Z"></path></svg>
Screen Shot 2023-06-28 at 3 42 18 PM

Maybe this one is intentional, since the lower/right bit isn't as visually "heavy" as the top/left bit. Not sure.

@jonbell-lot23
Copy link
Contributor Author

jonbell-lot23 commented Jun 28, 2023

(I should point out that I spent a lot of time measuring and eyeballing, but these shapes have so many optical differences (some are wider, some are taller, etc) that I am absolutely open to feedback about aligning them better.

So in the previous comment, I'm saying "it looks ok in Figma!" but also "but maybe something broke in the code, that wouldn't be 100% surprising to me"

[edit: I wrote this before noticing the comments above. Thanks for doing a run-through on the icons, my eyes have crossed from working on this so much, I appreciate the fresh eyes and will look into tweaks]

@jonbell-lot23
Copy link
Contributor Author

jonbell-lot23 commented Jun 28, 2023

Maybe this one is intentional, since the lower/right bit isn't as visually "heavy" as the top/left bit. Not sure.

Yup, that's exactly what happened. Mathematical alignment felt off, so this was fudged for optical alignment

Copy link
Collaborator

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

I'd prefer the style be moved, but this is otherwise ok.

<button
className="icon-with-tooltip-button"
className={styles.iconWithTooltipButton}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you move the style (styles.iconWithTooltipButton) to the icon, since it is what you're trying to style anyway? The IconWithTooltip component is only used in on place (Header) so you should be able to just do it there:

const backIcon = <div className="img arrowhead-right" style={{ transform: "rotate(180deg)" }} />;

@jonbell-lot23
Copy link
Contributor Author

Thanks for the review, Brian!

We talked about how you're the princess and the pea, and how important that is for us. ❤️

@bvaughn
Copy link
Collaborator

bvaughn commented Jun 29, 2023

We talked about how you're the princess and the pea, and how important that is for us. ❤️

Oh gosh! That is a good description 😅 😆

@jonbell-lot23
Copy link
Contributor Author

I'm having trouble moving the style and I'm trying to handle other things (breakfast, picking up Chris, etc) so I'm going to squash and merge and follow up on the style improvements in a different PR. I want as many eyeballs on the new icons today as possible, especially with us under one roof at the offsite.

@jonbell-lot23 jonbell-lot23 merged commit eac4a8a into main Jun 29, 2023
36 of 38 checks passed
@bvaughn bvaughn deleted the 2023-06-22-icon-refresh branch June 29, 2023 15:45
@jonbell-lot23
Copy link
Contributor Author

Here's the Linear ticket for a fast-follow, once I eat breakfast: https://linear.app/replay/issue/DES-748/follow-up-on-icon-pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants