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

feat: Add edge labels #1097

Merged
merged 1 commit into from
Jun 1, 2024
Merged

feat: Add edge labels #1097

merged 1 commit into from
Jun 1, 2024

Conversation

georgefst
Copy link
Contributor

@georgefst georgefst commented Nov 22, 2023

Behaviour is probably what we want, but implementation still a little messy.

We only display labels for let nodes for now. Eventually we might want to do more with this, including even labelling every single edge in beginner mode.

I had previously thought that labels should be on the outgoing connection rather than the edge itself, out of concern that the latter would look a bit like a node, and as an indication that the number of edges and their names is a property of the parent. But this design, with borderless labels, as iterated on with @dhess last week, actually looks alright.

Closes #24, I suppose.

Signed-off-by: George Thomas <[email protected]>
Signed-off-by: Drew Hess <[email protected]>
@dhess dhess marked this pull request as ready for review May 5, 2024 10:06
@dhess dhess added the Deploy service Deploy the backend service for this PR label May 5, 2024
@dhess
Copy link
Member

dhess commented May 5, 2024

This is rebased on main and seems to be working. Here's what I'd still like to do before merging:

  • Fix the typeface for the labels; it should be the same as our code font.
  • Tweak the styling of the translucent background a bit.
  • Lengthen the edges, at least for edges that have labels.
  • Investigate the TODO why? this seems like a bad abstraction comment.

I suspect that lengthening the edges might be tricky, so it might make more sense to punt that until later.

@dhess
Copy link
Member

dhess commented May 5, 2024

Screenshot of what this looks like at the moment:

Screenshot 2024-05-05 at 11 22 25 AM

@dhess
Copy link
Member

dhess commented May 5, 2024

These edge labels don't play nicely with one of our node label styles; e.g.:

Screenshot 2024-05-05 at 2 33 48 PM

It might be time to ditch the overlay-style labels and move to inline labels.

@dhess
Copy link
Member

dhess commented May 7, 2024

Upon further review, I think that rendering the edge labels identically to how we render their corresponding syntax nodes makes sense... at least for lets:

Screenshot 2024-05-07 at 7 33 44 PM

@dhess
Copy link
Member

dhess commented Jun 1, 2024

The code in this commit is maybe a bit rough, but I think it's good enough, and I don't want to hold up this feature any longer.

I'm pretty happy with rendering the edge labels as solid-filled "roundels," so I'm keeping that change.

I'm going to pass on the edge-lengthening bit for now.

@dhess dhess added this pull request to the merge queue Jun 1, 2024
Merged via the queue into main with commit 5c817ce Jun 1, 2024
34 checks passed
@dhess dhess deleted the georgefst/edge-label branch June 1, 2024 23:15
@dhess
Copy link
Member

dhess commented Jun 1, 2024

Ugh, turns out this branch did not include the roundel-style edge labels. I'll commit that change shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deploy service Deploy the backend service for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better rendering of lets
2 participants