-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
base: stratakit-css
Are you sure you want to change the base?
Add Sandcastle settings #12819
Conversation
Thank you for the pull request, @jjspace! ✅ We can confirm we have a CLA on file for you. |
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.
Looks good 👍 Just one actionable comment but otherwise good to go.
.info-badge { | ||
display: inline-block; | ||
|
||
.🥝-label > & { |
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.
Is text replacement from trying the write kiwi
referring to the library. This should be changed, right?
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.
This is already correct since we're using the stratakit components. This overrides the position anytime the badge is used inside a field label.
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.
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) { |
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.
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?
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.
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.
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.
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.
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.
Approved 👍
Not going to merge per @jjspace's request since this is dependent on another pending branch
Description
This PR:
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
npm run dev
inside the package ornpm run build-sandcastle
andnpm start
from the project rootAuthor checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change