Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Svelte: implement symbol redesign#62473

Merged
camdencheek merged 10 commits into
mainfrom
cc/symbol-kinds
May 9, 2024
Merged

Svelte: implement symbol redesign#62473
camdencheek merged 10 commits into
mainfrom
cc/symbol-kinds

Conversation

@camdencheek

@camdencheek camdencheek commented May 6, 2024

Copy link
Copy Markdown
Member

This implements the designs for the new symbol kind icons here.

screenshot-2024-05-07_16-51-12@2x
screenshot-2024-05-07_16-50-41@2x
screenshot-2024-05-07_17-03-31@2x
screenshot-2024-05-07_17-04-00@2x

Closes https://github.com/sourcegraph/sourcegraph/issues/61941

Test plan

Added storybook test. Manually tested the two places this was slotted in.

@cla-bot cla-bot Bot added the cla-signed label May 6, 2024
@camdencheek camdencheek marked this pull request as ready for review May 6, 2024 21:27
@camdencheek camdencheek requested review from a team and taiyab May 6, 2024 21:27
@taiyab

taiyab commented May 7, 2024

Copy link
Copy Markdown
Contributor

I checked out the branch here but didn't see any of the new symbol icons in search results, etc.

The icons visually look like they use actual text over being true outlined svgs? Apologies if this is wrong, I can't clearly see where the source for each individual icon is.

@camdencheek

Copy link
Copy Markdown
Member Author

new symbol icons in search results

Any chance you were running the React version?

The icons visually look like they use actual text over being true outlined svgs?

No, you're right. It is text, but it is text within an SVG. SVG supports rendering text in an image, and it's fully vectorized.

Did you want us to just copy the paths straight from the designs? I was thinking we wanted to parameterize these so we could easily adjust the short names, colors, spacing, etc. Totally fine if that's what you want, just misunderstood 🙂

@camdencheek

Copy link
Copy Markdown
Member Author

I can't clearly see where the source for each individual icon is.

The source for the SVG is here

@taiyab

taiyab commented May 7, 2024

Copy link
Copy Markdown
Contributor

Did you want us to just copy the paths straight from the designs? I was thinking we wanted to parameterize these so we could easily adjust the short names, colors, spacing, etc. Totally fine if that's what you want, just misunderstood 🙂

As long as it's strictly not adopting any styling from the stylesheets, then fine. To my eye I'm noticing some font-weight weirdness/faux bolding but could be wrong.

@taiyab taiyab left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@taiyab taiyab requested a review from a team May 7, 2024 16:53
@camdencheek

Copy link
Copy Markdown
Member Author

Converting to draft since we decided to just use the SVGs from Figma for now. We can't guarantee that our code font exists (it didn't on my system 🙃), so until we get fonts figured out, we're just going to use what we know looks good, which are the paths from Figma

@camdencheek camdencheek marked this pull request as draft May 7, 2024 20:46
@camdencheek camdencheek requested a review from taiyab May 7, 2024 20:52
@camdencheek camdencheek marked this pull request as ready for review May 7, 2024 20:52
@camdencheek

Copy link
Copy Markdown
Member Author

Alrighty, this is ready for review again

@taiyab taiyab left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Much better.

@taiyab taiyab requested a review from a team May 7, 2024 21:13
@taiyab

taiyab commented May 9, 2024

Copy link
Copy Markdown
Contributor

Excited about getting this in! :D

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we move --icon-color: var(--text-muted) on the top level, it seems we set it
both times for dark and light theme

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done!

@camdencheek camdencheek enabled auto-merge (squash) May 9, 2024 15:11
@camdencheek camdencheek merged commit d7dc2b9 into main May 9, 2024
@camdencheek camdencheek deleted the cc/symbol-kinds branch May 9, 2024 15:22
@taiyab

taiyab commented May 9, 2024

Copy link
Copy Markdown
Contributor

Quick note on this @camdencheek: The symbols weren't updated in places like the search suggestions:
CleanShot 2024-05-09 at 19 48 54@2x

There might've been other places missed out.

@camdencheek

camdencheek commented May 9, 2024

Copy link
Copy Markdown
Member Author

Oh weird. Those must use a different source for the icons because I deleted the code that should be handling that 😄 Will follow up 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Svelte: Implement new symbol kind icons

3 participants