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

Squiggle value menu additions #2564

Merged
merged 5 commits into from Nov 21, 2023
Merged

Squiggle value menu additions #2564

merged 5 commits into from Nov 21, 2023

Conversation

OAGr
Copy link
Contributor

@OAGr OAGr commented Nov 20, 2023

  • Added a "Log to console" menu option
  • Added "Unfocus" menu option, where relevant
  • Squiggle Output exports to window.squiggleOutput, for further debugging.
image

@OAGr OAGr requested a review from berekuk as a code owner November 20, 2023 17:18
Copy link

changeset-bot bot commented Nov 20, 2023

🦋 Changeset detected

Latest commit: e00fd23

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@quri/squiggle-components Patch
@quri/ui Patch
vscode-squiggle Patch
@quri/squiggle-lang Patch
@quri/prettier-plugin-squiggle Patch

Not sure what this means? Click here to learn what changesets are.

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

Copy link

vercel bot commented Nov 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 Nov 21, 2023 2:28am
squiggle-components ✅ Ready (Inspect) Visit Preview Nov 21, 2023 2:28am
squiggle-website ✅ Ready (Inspect) Visit Preview Nov 21, 2023 2:28am
1 Ignored Deployment
Name Status Preview Updated (UTC)
quri-ui ⬜️ Ignored (Inspect) Visit Preview Nov 21, 2023 2:28am

Copy link
Contributor

sweep-ai bot commented Nov 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 unnecessary separators or extra characters in code.
  • Apply: Use consistent indentation and spacing throughout the code.
  • Apply: Ensure that all code is properly formatted and follows the style guide.
  • Apply: Avoid using magic numbers or hard-coded values in the code.

Copy link
Collaborator

@berekuk berekuk left a comment

Choose a reason for hiding this comment

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

Great changes. window suggestion should be fixed before merge, and I don't care much about the second one, can be improved later.

packages/components/src/lib/hooks/useSquiggle.ts Outdated Show resolved Hide resolved
//Set the output to the window so that it can be accessed by users/developers there
//This is useful for debugging
if (window) {
window[WINDOW_VARIABLE_NAME] = output;
Copy link
Collaborator

Choose a reason for hiding this comment

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

There can be multiple Squiggle components on any page, so maybe this shouldn't be enabled by default?

Two alternatives:

  1. set this in onOutputChange prop that's passed to LeftPlaygroundPanel in SquigglePlayground (multiple playgrounds on a single page are rare)
  2. pass it to SquigglePlayground in onOutputChange prop (we don't have onOutputChange prop in the playground yet because we merged your version with onExportsChange before I implemented VersionedSquiggleChart, but I think we agreed to change it later); then log it in docs playground and in Squiggle Hub

Copy link
Contributor Author

@OAGr OAGr Nov 20, 2023

Choose a reason for hiding this comment

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

How about this:
We do (1), and also, we set each playground to have it's own ID, that we use for it.
Like, window.squiggleOutput.[Playground_ID]

We can generate the ID with https://react.dev/reference/react/useId.

If we do that, we could also save the project, sourceId, code, and anything else, if we want.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that's also an option.

OTOH, I've started to have doubts about the usefulness of this feature, compared to using built-in React dev tools:
Captura de pantalla 2023-11-20 a la(s) 15 39 09

I just tried it and it works; it's slightly hard to locate SquiggleViewer component because it doesn't have a name (I'm not sure why; we do forwardRef(function SquiggleViewer(...) {}) instead of forwardRef(() => ...), which should be enough), but other than that, it's quite convenient.

There's also right click -> "Store as global variable" action on individual props (hard to screenshot).

IIRC, until recently, Next.js obfuscated component names in production builds, but recently they changed it to look the same as in dev.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might not be very useful, but it seems pretty cheap. And the React dev tools are a browser extention - a fair bit more hacky.

This we could give to users for use.

Co-authored-by: Vyacheslav Matyukhin <me@berekuk.ru>
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