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

Tooltips for top-level variables #2776

Merged
merged 18 commits into from Jan 19, 2024
Merged

Tooltips for top-level variables #2776

merged 18 commits into from Jan 19, 2024

Conversation

berekuk
Copy link
Collaborator

@berekuk berekuk commented Dec 20, 2023

This PR also includes:

  • semantic tooltips for stdlib functions (doesn't show tooltips on normal in "normal string", etc.)
  • remove support for void values (())
  • AST cleanups (change names for nodes in Lezer to be similar to nodes in AST produced by Peggy)
  • highlighting fix for variables named with keywords (if = 5 is valid Squiggle but wasn't highlighed correctly by Lezer)

Copy link

changeset-bot bot commented Dec 20, 2023

⚠️ No Changeset found

Latest commit: 994ecad

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Dec 20, 2023

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

Name Status Preview Updated (UTC)
quri-hub ✅ Ready (Inspect) Visit Preview Jan 19, 2024 7:39pm
squiggle-components ✅ Ready (Inspect) Visit Preview Jan 19, 2024 7:39pm
squiggle-website ✅ Ready (Inspect) Visit Preview Jan 19, 2024 7:39pm
1 Ignored Deployment
Name Status Preview Updated (UTC)
quri-ui ⬜️ Ignored (Inspect) Visit Preview Jan 19, 2024 7:39pm

Copy link
Contributor

sweep-ai bot commented Dec 20, 2023

Apply Sweep Rules to your PR?

  • Apply: All docstrings and comments should be up to date.
  • Apply: Ensure that all variables and functions have descriptive names.
  • Apply: Avoid using magic numbers or hard-coded values in the code.

Copy link

codecov bot commented Dec 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5be13ee) 69.73% compared to head (ec3c210) 69.88%.
Report is 68 commits behind head on main.

❗ Current head ec3c210 differs from pull request most recent head 994ecad. Consider uploading reports for the commit 994ecad to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2776      +/-   ##
==========================================
+ Coverage   69.73%   69.88%   +0.14%     
==========================================
  Files         120      120              
  Lines        6791     6775      -16     
  Branches     1448     1444       -4     
==========================================
- Hits         4736     4735       -1     
+ Misses       2047     2032      -15     
  Partials        8        8              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@berekuk
Copy link
Collaborator Author

berekuk commented Dec 20, 2023

TODO (copy-pasted from Discord):

  • collapsible nodes don't work in the tooltip because <ViewerProvider> is not configured
  • need to pass settings from the playground, right now <CodeEditor> doesn't get settings and tooltips use defaultPlaygroundSettings
  • scroll, margins; maybe hide summaries
  • probably hide the gear icon

* main: (158 commits)
  Fixed some bugs that I'm surprised were ever there
  Merged with main
  Deleted unused file
  Minor refactors to SimpleValue
  Hot fix for method scale
  remove import assertion
  Added back scale default constants
  Added changeset
  ScaleShift -> Method
  Added back defaults to SqScale
  First pass on refactoring Scale
  convert textmate-grammar to a public package; highlight squiggle code in markdown
  Some simple JSON tests
  Moved JSON methods to Danger
  Cleaning up SimpleValue
  MarkdownViewer uses shiki instead of react-syntax-highlighter
  Fixed map issue
  Restructure to fix most tests
  Moving SimpleValue code
  Finishing touches to Tag.doc
  ...
@OAGr
Copy link
Contributor

OAGr commented Jan 9, 2024

Quick flag that it would be neat if this could work for imported variables. Like when someone types,

import "hub:ozzie/test" as hub

... you can hover over hub.

@OAGr
Copy link
Contributor

OAGr commented Jan 18, 2024

This still doesn't capture the fact that items named the same would get shown the last value, which seems pretty bad.

image

I'd wait until we can fix that.

That said, I also think this should wait for my PR about dots in the gutter.
https://squiggle-components-5lius8sjo-quantified-uncertainty.vercel.app/?path=/story/squiggle-squiggleplayground--comments

That might also help with choosing which are the last value.

@berekuk
Copy link
Collaborator Author

berekuk commented Jan 18, 2024

This still doesn't capture the fact that items named the same would get shown the last value, which seems pretty bad.
image

Ok, I added an extra check that the value's location matches the variable name: 2c5b2e1

So the tooltip just won't show on names that are shadowed later.

It would be preferable to show the temporary value, but that's hard. Showing a special tooltip (see

// TODO - if we can prove that the variable was shadowed, show the tooltip pointing to the latest assignment.
) would be better but that that's extra UI work, and I would prefer to spend time adding warnings to squiggle-lang instead.

return (
<TooltipBox view={view}>
<div className="px-4 py-1">
{/* Force a standalone ephermeral ViewerProvider, so that we won't sync up collapsed state with the top-level viewer */}
Copy link
Contributor

Choose a reason for hiding this comment

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

*ephemeral

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

Successfully merging this pull request may close these issues.

None yet

2 participants