-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix cut off examples #725
Fix cut off examples #725
Conversation
They have minus margin that makes the border getting cut off.
Seems to be a convention in other examples.
I'm really glad that we're fixing these examples, but I think that there might be another way we can handle it. MDX actually does this really cool thing where it parses everything after the language identifier in fenced code blocks as props and passes those down to the component that's delegated to render
...passes @simurai you might be able to test this theory by moving the |
Ohh.. 💯 yes, that would be the much better solution to keep things more "copy & paste friendly". |
This removes the gap at the bottom
This allows things like the focus ring to not get cut off.
Ok, I made this change 051ef7f and it kinda works:
But I couldn't figure out how to add multiple classes.. Maybe it's ok with just one class? I also moved the default padding inside the |
@simurai lemme look at how that's parsed in mdx... there might be a trick to it. 👀 |
Okay, so it looks from the source like the "metastring" (the part after the language identifier in a fenced code block) is parsed, but that it doesn't support quoted values. I used a module called parse-pairs in code-blocks, and it supported quoting both the key and value. So maybe either we file a PR for MDX to use that (or something similar)? |
After moving the default padding inside the iframe
This reverts commit 051ef7f.
Yeah, might be worth a try. 👍 Should we merge this PR already? Making a PR for MDX might still take a while. btw. I reverted 051ef7f because it removed the margin that wrapped the whole example. I think it comes from here: Line 71 in 645a61d
|
Yeah, let's merge this for now. I think we can also do this in our mdx loader with a remark transform, but I do think it'd be nice for mdx to support it out of the box. What would be really cool is if it supported just injecting the whole rest of the string as props in JSX, so that this worked:
|
Ok, I can add it to |
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.
Ack sorry, I meant to approve this earlier! 🚀
This is a follow up to #717. It fixes examples that get cut off due to:
The fixes are mostly:
.position-relative
They all are kinda 😩 hacks, but at least it looks less broken.☺️
✨ Preview: https://primer-css-fix-cut-off-examples.now.sh/css
/cc @primer/ds-core