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.

Sign up for GitHub

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

Jump to bottom

Bring node labels in-line #1005

Merged
merged 5 commits into from
Aug 3, 2023
Merged

Bring node labels in-line #1005

merged 5 commits into from
Aug 3, 2023

Conversation

georgefst
Copy link
Contributor

@georgefst georgefst commented Jul 5, 2023 •
edited
Loading

At a recent in-person meeting, @dhess mentioned wanting to move labels to the left, and after thinking about it I said I'd like to try a bigger change, with labels brought down to the height of the main node contents. This is a mock-up of that change.

Pros:

  • Reads nicely (more text-like) for λ and ∀ nodes.
  • Allows us to make better use of space on the canvas, since the more uniform shape of nodes means we don't need to worry about labels overlapping with edges or other nodes.
  • I personally just much prefer it on a base aesthetic level - it feels cleaner.

Still to do:

  • Consider rendering of beginner-mode syntax nodes.
    • Revisiting this a few weeks later, I'm not totally sure what I considered the problem to be. I guess it's that the label and contents blur together due to both having solid backgrounds? If so, I don't think it's a major issue.
  • Fix the sometimes-visible gaps (anti-aliasing?) between labels and the node border.
    • I really don't know how to fix this. And I don't want to sink any more time in to it when this style is just an experiment which we might not keep around.
  • Flesh out commit messages.
  • Stuff that's related, but can be tackled separately:
    • Revisit our set of labels, e.g. Var and V, especially given our terminology discussions.
    • Be consistent: show labels on all type def nodes, and definition name nodes.

@georgefst georgefst added the Deploy service Deploy the backend service for this PR label Jul 5, 2023
@dhess
Copy link
Member

dhess commented Jul 6, 2023 •
edited
Loading

I don't think this is a bad design, but I think it has some problems that the current design doesn't:

  • For non-syntax nodes, we lose quite a bit of horizontal space that would otherwise be available for names, so either we'll end up truncating more names, or we'll need to make the non-syntax nodes wider than they are in the current design.
  • One idea we discussed for higher levels (intermediate, expert, etc.) was to only show node labels on hover and/or when toggled. That works nicely with the current design, but not as well with this one, since it would mean shifting the name to the right (or expanding the node width, which is a non-starter IMO).
  • This style doesn't work very well for patterns labels (P), which I think we should add for uniformity with other nodes.
  • Variable nodes now look uncannily like pharmaceutical capsules (i.e., medicine), which is a bit odd.

So overall, IMO, this is a net negative change. I think the only win is that it more closely resembles what you'd see in text mode.

@dhess
Copy link
Member

dhess commented Jul 6, 2023

How much extra work would it be to support this style and the current style in a sort of theming system?

@georgefst
Copy link
Contributor Author

How much extra work would it be to support this style and the current style in a sort of theming system?

Good idea. Not very much work since the diff here is pretty small. I've just pushed a proof-of-concept in the last commit. It's not toggleable at runtime, but there's a storybook example using it, and it could be used for the app with a one-line change.

For me, only your second bullet point is a major issue, and I'd like to discuss alternatives there as I don't think it's something we've ever discussed in depth. So I like the idea of keeping this around as an option to experiement with.

@dhess
Copy link
Member

dhess commented Jul 6, 2023

OK, then, feel free to clean this up and get it ready for merging. For the time being, we could add a 3rd state to the current tree/text button that selects which of the tree styles is active. (Eventually we will need to think about whether that's the right place for it — we'll undoubtedly have other theming support in the longer run and could put the tree style in whatever settings pane we create for that.)

@georgefst georgefst marked this pull request as ready for review July 6, 2023 16:21
@georgefst georgefst requested a review from a team July 27, 2023 17:53
@dhess
Copy link
Member

dhess commented Jul 31, 2023

I'd like to review this, but the deployment failed.

@georgefst
Copy link
Contributor Author

The CI failure looks unrelated, since this PR doesn't change any dependencies.

@dhess
Copy link
Member

dhess commented Jul 31, 2023

I see, looks like the audit failed, perhaps because some new vulnerabilities have been found since we last built. I'll look into it.

@georgefst georgefst force-pushed the georgefst/inline-labels branch 2 times, most recently from 0cd5979 to e4b3ca9 Compare August 2, 2023 15:19
@georgefst
docs: Fix comment typo
57b2fc1 
Signed-off-by: George Thomas <[email protected]>
@georgefst
feat: Add optional "inline" mode for displaying nodes
647c748 
This uses up less space, and reads nicely (more text-like) for `λ`/`∀`/`Λ` nodes.

We can trigger this mode through the tree/text toggle. This is a slight abuse of this element, and we'll likely revisit this if/when we have more optional display behaviour. On the other hand, this display option is fairly experimental for now, and it may well be that we don't keep both tree styles around in the long run.

Signed-off-by: George Thomas <[email protected]>
@georgefst
Copy link
Contributor Author

I've force-pushed some simple fixes here: truncation of overlong names, a storybook type error causing weird toolbar rendering, and an oversight where spacing changes were being applied regardless of the mode.

@georgefst
fix: Avoid anti-aliasing issues in "inline"-style nodes
ddf7881 
We had an issue where there were sometimes small visible gaps between the node label and border. These gaps would flicker while zooming in and out. This was seen across multiple browsers.

We fix this by adding extra padding around the label so that it intersects the border, rather than meeting it precisely.

There is a side effect here, which is that labels in term-level nodes are now curved on both sides (I wouldn't have known how to implement this otherwise). But this is arguably an improvement anyway.

Signed-off-by: George Thomas <[email protected]>
@georgefst
chore: Formatting
284a2fa 
Signed-off-by: George Thomas <[email protected]>
@dhess
ci: disable pnpm audit step for the time being
d2987d0 
`pnpm audit` is currently complaining about the following GitHub
security advisories on the `vm2` package:

GHSA-cchq-frgv-rjh5
GHSA-g644-9gfx-q4q4

We don't use `vm2` directly: it is a (very) transitive dependency of
`ibm-openapi-validator` via `@orval/core`. The validator is only
potentially run when we generate our Primer API bindings, and in fact,
we do not even currently run the validator at all, as it doesn't like
our OpenAPI spec. Therefore, this advisory isn't relevant to us at
this time.

Unfortunately, there is currently no way to tell `pnpm audit` to
ignore GHSA advisories, only CVEs. See relevant discussion in:

https://github.com/orgs/pnpm/discussions/6204
pnpm/pnpm#6838

Ironically, it seems like it may be possible to use another IBM tool
to post-process the output of `pnpm audit` and ignore certain issues:

https://github.com/orgs/pnpm/discussions/5229

However, until we have time to look into that, we'll just disable
`pnpm audit` in order to make forward progress. Note that we are also
running regular security scans via GitHub's Dependabot and Snyk, so we
have good security coverage even without the `pnpm audit` step.

Signed-off-by: Drew Hess <[email protected]>
@dhess
Copy link
Member

dhess commented Aug 3, 2023

I have disabled the CI pnpm audit step in order to make forward progress. This is not as foolish as it may seem: see commit comment on d2987d0 for details.

@dhess dhess added Deploy service Deploy the backend service for this PR and removed Deploy service Deploy the backend service for this PR labels Aug 3, 2023
dhess
dhess approved these changes Aug 3, 2023
View reviewed changes
Copy link
Member

@dhess dhess left a comment

Choose a reason for hiding this comment

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

tree 1 and tree 2 obviously need new names, but this is fine for now.

I still have reservations about the inline label UI (see earlier comment), but we can let students decide now when we start testing.

@dhess
Copy link
Member

dhess commented Aug 3, 2023

(I will also note that this PR doesn't actually move labels to the left, which was my original suggestion that started the whole thing off 😂)

@georgefst georgefst added this pull request to the merge queue Aug 3, 2023
Merged via the queue into main with commit debb33d Aug 3, 2023
6 checks passed
@georgefst georgefst deleted the georgefst/inline-labels branch August 3, 2023 12:14
@georgefst
Copy link
Contributor Author

I don't think this is a bad design, but I think it has some problems that the current design doesn't:

We discussed these informally in a meeting at some point, but for the record:

For non-syntax nodes, we lose quite a bit of horizontal space that would otherwise be available for names, so either we'll end up truncating more names, or we'll need to make the non-syntax nodes wider than they are in the current design.

The new mode has wider nodes, and less spacing between nodes. There should be a net gain of space, since the packing of nodes is now simpler due to their uniform shape, as discussed in OP.

One idea we discussed for higher levels (intermediate, expert, etc.) was to only show node labels on hover and/or when toggled. That works nicely with the current design, but not as well with this one, since it would mean shifting the name to the right (or expanding the node width, which is a non-starter IMO).

This is an interesting point. I do think, in general, we should treat syntax labels (λ/let) differently to those which are more like annotations (V/Var). But I don't have an overall design in mind here yet. One thing to consider is that we will want the former to appear in text mode, while the latter likely won't.

This style doesn't work very well for patterns labels (P), which I think we should add for uniformity with other nodes.

I'm not sure. Pattern nodes are really quite different to others. Anyway, we could easily shove a label in the top-left.

Variable nodes now look uncannily like pharmaceutical capsules (i.e., medicine), which is a bit odd.

I understand the concern, but this doesn't really bother me personally. I've also just noticed that it's actually far less the case since ddf7881.

@georgefst georgefst mentioned this pull request Aug 3, 2023
Merged
@georgefst
Copy link
Contributor Author

(I will also note that this PR doesn't actually move labels to the left, which was my original suggestion that started the whole thing off 😂)

I wasn't sure if this PR was the right place to make that change. Anyway, it's trivial: #1016.

@dhess
Copy link
Member

dhess commented Aug 3, 2023 •
edited
Loading

Sorry, it was just an amusing aside. You’re right that it’s best not to mix two unrelated changes in a single PR.

This was referenced Aug 9, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhess dhess dhess approved these changes

Assignees
No one assigned
Labels
Deploy service Deploy the backend service for this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@georgefst @dhess