-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
website: components: add multilinecodeblock component #13179
base: main
Are you sure you want to change the base?
website: components: add multilinecodeblock component #13179
Conversation
✅ Deploy Preview for authentik-storybook canceled.
|
✅ Deploy Preview for authentik-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #13179 +/- ##
==========================================
+ Coverage 92.73% 92.75% +0.01%
==========================================
Files 794 794
Lines 40419 40419
==========================================
+ Hits 37484 37490 +6
+ Misses 2935 2929 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Thanks @dominic-r for this PR. As we discussed, I am torn about adding this much code simply to support my desire to able to use italic font within a code block (though yes we do need do be able to do that for our Style Guide rule about variables being italicized). Pros (as I understand):
Cons (as I understand):
@BeryJu @kensternberg-authentik and @GirlBossRush can we get your opinion on this? I am leaning yes (despite my dislike of magic haha), but want to hear from you all. |
I do have to make one correction: This only affects multiline code blocks such as your env example, it does not replace one-line code blocks Adds mabye 110-120 lines to the CSS file, not 300. |
How to use this component is explained in the new style guide PR that will be merged soon. https://github.com/goauthentik/authentik/pull/12929/files#diff-2f46a3811176c0ff14ad45113db1f50d5a801aa013071674e5b9f7d437705bc4R125 (I have to modify import path as it changed, other than that it gives a small overview). Same "magic" as tabs. For tabs, Half of your code just disappears in a tab you can't see till you click it. So, this is less magical |
Haha mdx tabs aren't magical, are standard OoB, every writer knows how to use them. But good to hear you are also updating the Style Guide in how to use this. Thanks! |
I'm not particularly worried about the size of the React or the CSS; the CSS, in fact, looks very reasonable: the block, the button, the code itself, and the "copy to clipboard" button are all it references. I wonder if I should be concerned that the CSS for things like color and font size are hard-coded; I don't know what sort of theming consequences that has for the future. The code itself is very well-written and very readable, especially for React. Tana's complaint that this is a "teaching the documentation writers how to use it" issue is valid, but I think can be well-handled in our documentation about documentation (how meta!). If there are use cases requiring posting multi-line code blocks, I'd approve this and see what the consequences were. My complaint, if I have one, is that I'm not sure there should be styling in a code block, and I'm not sure if you want that styling showing up in the display (what if the code block to be displayed is HTML, after all!?), and I'm definitely not sure you want the styling tags getting copied with the copy button, but that's what looks like happens. |
Thanks!
True!
There are quite a few actually, several integration docs have environment variables the user needs to put in an .env file, json payload, or other.
If we were to keep variables in italic to follow the style guide, how else would we indicate what's to replace? I have personally seen issues where users did not know a specific part of a string was a placeholder a few times, and I feel it would be beneficial to make it clear to the user.
True. Tho, I feel like we need to evaluate the use case. I personally haven't seen any integration docs which use customizable HTML. If that dreadful moment happens, I can look for other solutions (or your previous point ^).
Hmmm I'll investigate. I remember fixing that issue in my last PR.. |
Thanks @kensternberg-authentik for your excellent points, and @dominic-r for your excellent counter-points. OK, after those, my only remaining concern would be the copying of the formatting issue. |
Signed-off-by: Dominic R <[email protected]> wip Signed-off-by: Dominic R <[email protected]> wip
625070e
to
6f7cf7e
Compare
website/src/css/custom.css
Outdated
/*#region Integration Guide Codeblocks*/ | ||
|
||
/* Base styles */ | ||
.integration-codeblock { |
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.
would it be possible to make this a styles.module.css and import it in the component versus adding it to the global CSS?
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.
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.
like 2a27510; there are some other options that can be done with this to namespace the CSS but we dont have to do that for this
Signed-off-by: Jens Langhammer <[email protected]>
What?
This adds a component which renders a multiline codeblock in a way which allows usage of tags such as
<em>
while minimizing boilerplate code. Here's a rendered version of the component:This will be needed for future documentation PRs and is part of a larger task.
Originally part of #12716, but extracted and modified to get this merged first for time constraints
Testing
Create a file in the website directory with the following content to test functionality. Add tags such as
<script>
ensure they are not rendered in the documentation UI. Add<em>
to ensure content is italicized correctly.Details
REPLACE ME
Checklist
ak test authentik/
)make lint-fix
)If an API change has been made
make gen-build
)If changes to the frontend have been made
make web
)If applicable
make website
)