Skip to content

Add Sandcastle settings #12819

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 7 commits into
base: stratakit-css
Choose a base branch
from
Open

Add Sandcastle settings #12819

wants to merge 7 commits into from

Conversation

jjspace
Copy link
Contributor

@jjspace jjspace commented Aug 14, 2025

Description

This PR:

  • Implements Sandcastle settings using local storage
    • Theme (which already existed)
    • Editor font + ligature option
      • I included 3 popular code editor fonts for now, adding more is easier. Droid Sans is the "default" for monaco even though they don't bundle it
    • Default starting panel, gallery or editor
      • When loading a specific sandcastle from the URL, named or a share link, we always default to the editor
      • If not loading a specific sandcastle we fall to the setting
      • The setting will default to Gallery for new users
      • This is meant as a convenience factor for more regular/power users
  • Implements dialogs for sandcastle using stratakit variables for consistent style and uses this for the settings modal

I meant to also include the popovers for the sandcastle metadata and share interaction in this PR but opted to do them in a follow up to keep the PR more focused and easier to review.

Issue number and link

part of #12566

Testing plan

  • Run sandcastle using npm run dev inside the package or npm run build-sandcastle and npm start from the project root
  • Play with the new settings modal
  • Make sure settings are preserved after refresh
  • Make sure the editor font matches
  • Make sure changing the theme from the sidebar and within the modal both work and don't conflict
  • Make sure the default starting panel works as expected

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 review from ggetz and lukemckinstry August 14, 2025 18:25
Copy link

Thank you for the pull request, @jjspace!

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

Copy link
Contributor

@lukemckinstry lukemckinstry left a comment

Choose a reason for hiding this comment

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

Looks good 👍 Just one actionable comment but otherwise good to go.

.info-badge {
display: inline-block;

.🥝-label > & {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is text replacement from trying the write kiwi referring to the library. This should be changed, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already correct since we're using the stratakit components. This overrides the position anytime the badge is used inside a field label.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok thanks for explaining the backstory offline that the strata codebase uses emojis for class names

margin-top: var(--stratakit-space-x4);
}

@media (min-width: 500px) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The modal looks good on mobile and responsive to page resizes. The main page (viewer and gallery/editor) do not seem optimized for smaller screens yet, is that coming in a future PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Longer term we've discussed making sandcastle "look nice" on mobile even if it's not very functional. However this is not a focus for the first version.
These styles were based on the example from the AriaKit Dialog sample code which already included this so I just kept it. It's especially good because this is a very generic component so when we do take a pass for mobile styles this is already ready to go.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good re: the modal

I know it is outside the scope of this PR, but for general mobile support for the new sandcastle, I am in favor of some basic support to view at least code right off the bat, rather than following up later, if that is reasonable. I tried viewing in mobile using developer tools and it was not usable.

@lukemckinstry lukemckinstry self-requested a review August 19, 2025 19:48
Copy link
Contributor

@lukemckinstry lukemckinstry left a comment

Choose a reason for hiding this comment

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

Approved 👍

Not going to merge per @jjspace's request since this is dependent on another pending branch

@jjspace jjspace mentioned this pull request Aug 22, 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.

2 participants