Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sgammon
Copy link
Contributor

@sgammon sgammon commented Sep 11, 2023

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.

Before: After:
Screenshot 2023-09-11 at 2 05 48 PM Screenshot 2023-09-11 at 2 06 00 PM

When the Customize this snippet area is expanded, controls are revealed. Checking the dev_dependency box updates the snippet accordingly:

Screenshot 2023-09-11 at 2 07 35 PM

Changelog

  • feat: add controls for code snippet customization
  • feat: add dev_dependency code snippet control
  • feat: add syntax highlighting in CopyCode component
  • fix: linebreak formatting in code snippet
  • chore: add prism and prism-themes dependencies

- 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]>
@sgammon sgammon force-pushed the feat/code-snippet-fmt branch from 8abdd4d to 45286f0 Compare September 11, 2023 21:15
Copy link
Member

@hobofan hobofan left a 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.

Comment on lines +33 to +39
const [codeSample, setCodeSample] = useState<string>(
codeTemplate(module, version, isDevDependency)
)

useEffect(() => {
setCodeSample(codeTemplate(module, version, isDevDependency));
}, [isDevDependency]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Copy link
Member

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.

Comment on lines +8 to +15
pre.codesample {
text-align: left;
background: transparent;
}

button pre.codesample, button pre code[class*="language-"] {
color: #6E6E6E;
}
Copy link
Member

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.

@hobofan hobofan self-requested a review October 4, 2023 18:13
Comment on lines +33 to +39
const [codeSample, setCodeSample] = useState<string>(
codeTemplate(module, version, isDevDependency)
)

useEffect(() => {
setCodeSample(codeTemplate(module, version, isDevDependency));
}, [isDevDependency]);
Copy link
Member

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.

@sgammon
Copy link
Contributor Author

sgammon commented Oct 4, 2023

@hobofan thank you for the review :) will fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants