-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Always switch to editor when it should #12820
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
base: main
Are you sure you want to change the base?
Conversation
Thank you for the pull request, @jjspace! ✅ We can confirm we have a CLA on file for you. |
I notice when I click the "open code" button for a sandcastle that is already open, it reloads the scene in the viewer. This seems unnecessary. Can we skip this? I see toggling the "gallery" and "editor" buttons does not reload the scene in the viewer, which seems better. |
This was intentional to keep the behavior consistent. If you click the card for a gallery item it will load it even if it is already loaded. I meant this to be a way to "re-run" without switching to the code panel. I think this is good. (The "active" detection is a little messy now relying on the url. I plan to change this a bit later based on some other open PRs but not in this one to avoid conflicts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go once CI is passing
@lukemckinstry I've merged in the changes to prevent leaving with pending changes. I believe this should be good to merge now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jjspace I want to clarify the behavior here, as per my comment below.
Otherwise, I don't see any issues with the code.
packages/sandcastle/src/App.tsx
Outdated
} | ||
setLeftPanel("editor"); | ||
} else { | ||
// Load the gallery item every time it's clicked ro act as a "rerun" button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Load the gallery item every time it's clicked ro act as a "rerun" button | |
// Load the gallery item every time it's clicked to act as a "rerun" button |
But moreover, I agree with Luke that this seems like confusing behavior.
This was intentional to keep the behavior consistent. If you click the card for a gallery item it will load it even if it is already loaded. I meant this to be a way to "re-run" without switching to the code panel. I think this is good.
Why do we need to rerun from the gallery list at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this behavior there is (currently) no way to "re-run" or reset a sandcastle without first switching to the code view. I don't know about others but I personally find I often want to reset sandcastles if I've moved the camera or edited values and just want a quick way to get back to the "start". Maybe we need to move the Run
button or duplicate it but for now this solution felt like a good compromise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ggetz did you have thoughts around this?
Possibly replaced by some of the logic in #12854, revisit later |
Description
Fixes a bug when you have a specific sandcastle loaded and click the "Open code" button on that same sandcastle in the gallery
Issue number and link
Part of #12566
Testing plan
npm run dev
in the package ornpm run build-sandcastle
andnpm start
at the project rootAuthor checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change