-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: formatting of code snippet #105
base: main
Are you sure you want to change the base?
Conversation
- feat: format `bazel_dep` code sample with line breaks - chore: cleanup main app name (`MyApp` became `BazelCentralRegistry`) - chore: add `prism` and `prism-themes` deps for syntax highlight Signed-off-by: Sam Gammon <[email protected]>
8abdd4d
to
45286f0
Compare
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.
LGTM! Some small nits, but would still accept the PR as-is.
const [codeSample, setCodeSample] = useState<string>( | ||
codeTemplate(module, version, isDevDependency) | ||
) | ||
|
||
useEffect(() => { | ||
setCodeSample(codeTemplate(module, version, isDevDependency)); | ||
}, [isDevDependency]); |
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.
const [codeSample, setCodeSample] = useState<string>( | |
codeTemplate(module, version, isDevDependency) | |
) | |
useEffect(() => { | |
setCodeSample(codeTemplate(module, version, isDevDependency)); | |
}, [isDevDependency]); | |
const codeSample = codeTemplate(module, version, isDevDependency) |
As that's a very simple calculation of a computed value, we can also just due it inline during rendering and keep the flow of hooks simpler
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.
Oh, I just saw that the useEffect
is missing module
and version
in it's dependency array, which causes the wrong code snippet to be displayed after a page navigation between modules.
pre.codesample { | ||
text-align: left; | ||
background: transparent; | ||
} | ||
|
||
button pre.codesample, button pre code[class*="language-"] { | ||
color: #6E6E6E; | ||
} |
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.
Not sure if the code snippet could be styled here, or if that would be more complicated, but overall the new code snippet has worse contrast (gray on gray), which isn't great for accessibility reasons.
const [codeSample, setCodeSample] = useState<string>( | ||
codeTemplate(module, version, isDevDependency) | ||
) | ||
|
||
useEffect(() => { | ||
setCodeSample(codeTemplate(module, version, isDevDependency)); | ||
}, [isDevDependency]); |
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.
Oh, I just saw that the useEffect
is missing module
and version
in it's dependency array, which causes the wrong code snippet to be displayed after a page navigation between modules.
@hobofan thank you for the review :) will fix |
Summary
This changeset fixes formatting of the Bazel module code snippet, and adds syntax highlighting with Prism. Additionally, the snippet can be customized to show as a
dev_dependency
.When the
Customize this snippet
area is expanded, controls are revealed. Checking thedev_dependency
box updates the snippet accordingly:Changelog
dev_dependency
code snippet controlCopyCode
componentprism
andprism-themes
dependencies