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

Better handling of multiple code blocks + code snippet styling #129

Merged
merged 19 commits into from
Jun 12, 2020

Conversation

jerelmiller
Copy link
Contributor

Description

Better handles guides that use multiple code blocks in a single step. This also addresses some styling issues with the code snippet that caused the page to horizontally scroll.

Reviewer Notes

Helpful link: https://pr-129.dlyi50rq9kt6c.amplifyapp.com/build-apps/build-a-custom-app/

Related Issue(s) / Ticket(s)

If there are any related GitHub Issues or JIRA tickets, add links to them here.

Screenshot(s)

Screen Shot 2020-06-11 at 12 47 53 PM

Screen Shot 2020-06-11 at 12 47 58 PM

Screen Shot 2020-06-11 at 12 48 06 PM


const CodeSnippet = ({ children, copy, className, lineNumbers }) => {
const language = className.replace('language-', '');
const [copied, setCopied] = useState(false);
const formattedCode = useFormattedCode(children ?? '');
const [copied, copyCode] = useClipboard();

return (
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

even though this isn't a change, is there a reason to use <div> instead of <React.Fragment>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I'll defer to @zstix or @LizBaker on this. I suspect its because we don't want containers outside this div to accidentally modify the layout (e.g. such as a display: flex since this component renders the code block + the copy bar.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that we did this to prevent flex from messing with the code snippet stuff, but I'm not 100% sure (was a week or so ago).

@@ -21,11 +34,6 @@ SideBySide.propTypes = {
children: PropTypes.node.isRequired,
type: PropTypes.string.isRequired,
className: PropTypes.string,
dir: PropTypes.oneOf(['right', 'left']),
Copy link
Contributor

Choose a reason for hiding this comment

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

I did not know of this PropTypes function but it is useful :)


const useClipboard = ({ duration = 1000 } = {}) => {
const [copied, setCopied] = useState(false);
const copy = useCallback((text) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

why use useCallback here and not useMemo ? What is the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question! useCallback is actually a shorthand for useMemo that returns functions. Per the React docs:

useCallback(fn, deps) is equivalent to useMemo(() => fn, deps)

@zstix zstix added bug Something isn't working enhancement New feature or request labels Jun 11, 2020
Copy link
Contributor

@timglaser timglaser left a comment

Choose a reason for hiding this comment

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

It'd be great to update the example & doc on how to provide multiple code blocks and the paragraphs to go with to reduce any uncertainty.

@jerelmiller
Copy link
Contributor Author

@timglaser addressed!

@nr-opensource-bot
Copy link
Contributor

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants