Skip to content

Solidify new Sandcastle React structure #12639

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

Open
wants to merge 16 commits into
base: nested-gallery
Choose a base branch
from

Conversation

jjspace
Copy link
Contributor

@jjspace jjspace commented May 29, 2025

Description

When I originally threw this together for the hackathon I tried to reuse a lot of the existing code for current sandcastle to speed up development. This meant a lot of the logic was very "imperative" oriented and was not playing nicely with React. This worked fine for a while but started to really get in the way in #12631, specifically with multiple loading methods.

This PR restructures the application to be much more "React" oriented

  • State is now much more actively maintained and controlled
    • This prevents all the direct monaco editor getValue and setValue access I was doing before
  • The iframe wrapper logic has been pulled out to it's own Bucket
    • "Running" a sandcastle is now controlled by changing the props on the Bucket using the comittedCode and runNumber state
  • Unified into a single Monaco Editor instance using multiple Models. It is now a controlled component and the contents are tracked and stored in state

This code builds off of #12631 and should be merged after that

Issue number and link

Part of #12566

Testing plan

  • Run sandcastle using npm run dev from inside packages/sandcastle
  • Load sandcastle at http://localhost:5173/
  • Make sure that:
    • Loading from a share URL (like this) works and has the correct code and viewer state on first load
    • Clicking gallery items loads the correct code, refreshes the viewer and updates the URL
      • Clicking the same item again should reload that item but not create an extra entry in history. test with back/forward buttons in browser
    • Clicking New resets both the JS and HTML code, refreshes the viewer and updates the URL to the base route
    • Navigating to a "legacy url" like http://localhost:5173/?src=3D%20Models.html works and replaces the url with the new id
    • Swapping between JS and HTML works as expected
    • The Add button and others work but do not refresh the viewer until Run is clicked
    • Hitting F8 while focusing inside the code actually reloads the viewer
    • Clicking Run or F8 multiple times in a row with no code changes refreshes the viewer every time
      • This is specifically why I added the runNumber state, this is a workflow that must work

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

@jjspace jjspace requested a review from ggetz May 29, 2025 14:39
Copy link

Thank you for the pull request, @jjspace!

✅ We can confirm we have a CLA on file for you.

@jjspace
Copy link
Contributor Author

jjspace commented May 30, 2025

@aruniverse or maybe @angrycat9000, would one (or both?) of you have some free time to take a look at this PR and review the React usage? No need to analyze the Sandcastle structure or "business logic". I haven't done much React dev in a while and I'm trying to get used to the mental paradigms again. I just want to make sure I'm not doing anything really horrible.

Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

I'm certainly not an expert, but the react updates here are looking good to me. The reducer looks like the right level of state management we're looking for at this stage.

Depending on how deep we go with components, we may need to look into incorporating contexts, but that seem to be excessive for the current state of things.

background-color: white;
background-image: var(--_rings), var(--_gradient);

/* TODO: these should pull from the stratakit directly somehow eventually */
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove the TODO's please and get them documented in a GitHub comment?

Comment on lines 414 to 416
// TODO: I like this method for history but we need to actually react to the user
// hitting forward or back, right now it does nothing. Need to look into how that works
// Might need to involve React-Router but I'd like to try and avoid if we don't need it
Copy link
Contributor

Choose a reason for hiding this comment

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

What the plan here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought this would be more complicated than it was but I've hooked up to the browser popstate events to listen and respond to navigation which makes it work nicely.

@angrycat9000
Copy link
Contributor

Did a read through of the code. The sandcastle v2 link didn't work for me so I wasn't sure how to run it.

The DOM manipulation in Bucket raised a red flag for me. Would have expected that everything is written in React and not setting innerHTML. If we do need to do the direct manipulation then I would clearly document why it is needed.

Otherwise looks pretty standard.

Would also recommend breaking stuff down into smaller components for readability. The App component is a bit hefty and has a lot of logic that could probably be better isolated. Eg the editor set/teardown could be in its own Editor component. The gallery loading could be in its own component that uses "onGallerySelected" to communicate back to the main app.

@jjspace
Copy link
Contributor Author

jjspace commented Jun 16, 2025

Thanks for the review @angrycat9000

The DOM manipulation in Bucket raised a red flag for me. Would have expected that everything is written in React and not setting innerHTML. If we do need to do the direct manipulation then I would clearly document why it is needed.

The only DOM manipulation we're still doing is to modify/control the contents of the iframe, ie the "bucket", to load the sandcastle code. AFAIK there's not a way to do that more directly using React. I moved all that code into the Bucket file specifically to have it all isolated into that single component.

@jjspace
Copy link
Contributor Author

jjspace commented Jun 16, 2025

@ggetz this PR has been cleaned up a bunch and should be good for another look

@jjspace jjspace mentioned this pull request Jun 18, 2025
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants