-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Stratakit css cleanup #12802
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
Stratakit css cleanup #12802
Conversation
Thank you for the pull request, @jjspace! ✅ We can confirm we have a CLA on file for you. |
Totally can be a follow-up action—and we should check with the stratakit designers—but I might suggest styling the slider track consistently with the loading bar by matching colors, removing outlines, and adjusting width. ![]() ![]() |
I agree that a sandcastle (or a couple sandcastles, depending) just demonstrating how to use the Sandcastle API would be useful for testing, but would also be good for the sake of documentation as well. |
Low priority, so please add it to end the running list if you agree: I just noticed that the Insert > UI element command adds the snippet to the end of the document rather than where the cursor currently is. I would expect it to be added where the cursor is, like how copy/paste works. |
Also low priority to add at the bottom of the list: I noticed the "Loading..." message bounces around as other elements in Sandcastle load. I think we should try to avoid that if possible. |
Some naming suggestions for clarity and future-proofing:
Is the term "mimic" used commonly? I might suggest omitting it, or using something more common like "polyfill". I would suggest moving the "directly sourced" css to Then I would keep the custom adjustments in |
Thanks @jjspace! That's all from me for now 😅 |
Thanks @ggetz, some responses for you:
I set the slider coler to the primary accent color for stratakit. This matches "accented" buttons and the switch toggle. I think the more vibrant glow color would be a bit too aggressive. The way I did it now is also relying on the native element so we don't have to implent custom slider elements. We can revist if we want later but I'm going to leave as is for now.
As you said most of the differences here are because all sandcastles that used the toolbar specified their own styles for a long time. Happy to include checking for this in the content pass but I don't think it's necessary for this PR
Noted. It's a lot easier to add it at the end since we don't have to interact with the monaco editor at all. I'll add it to the list to look into later, maybe it's not too hard.
I made many edits trying to combat this as much as possible. I thought I had finally quashed it from happening... I don't see it myself anymore but will add it to the list of bugs to look into later
I don't know if it's common or not but I think it describes what it's there for pretty well. They're not polyfills and I don't want to omit it as that could cause confusion with the actual stratakit elements.
The problem is these files are not distributed with the stratakit packages directly. That's part of the whole frustration in trying to use them outside the react setup. |
I do see your point, but now with the component styling updates the contrast is really low which makes it a usability issue IMO. I would like to see a content update to fix specifically that background color imminently, if not in this PR. |
Thanks for looking into this. I agree it's probably not worth the time sink right now. |
Fair. My concern was mostly around making a clear distinction between what we're doing specifically to duplicate stratakit versus our own adjustments. I think putting them in different locations is the clearest way to do so, but let me know if disagree with that point. |
@ggetz Per our discussion yesterday I've centralized the toolbar styles to make the background consistent. I put it in a separate PR just to make the review a little easier: #12848 |
Thanks for addressing these @jjspace! |
Description
This pr tackles a few different things but all focused around Sandcastle's CSS and stratakit
unstable_loadStyles
function from stratakit to initialize all css variables from stratakit inside non-react pages.stratakit-mimic-*
css components to style components in the viewer bucket and standalone pages to match the stratakit styles.public/styles/stratakit-mimic/components
are sourced directly from stratakit with minor changes to the class names to avoid naming conflicts. This was done to reduce duplicated work and make sure we match the styles as close as possible<button>
to allow matching styles without requiring people to know the classes or html structureSandcastle
API to match the new class names and required structureIssue number and link
Part of #12566
Testing plan
npm run dev
from the sandcastle package ornpm run build-sandcastle
andnpm start
from the repo rootSandcastle
API for inputsSandcastle
API for inputs, both should look goodAuthor checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change