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

DOP-1604: Allow code blocks to render with caption #299

Merged
merged 5 commits into from
Oct 19, 2020

Conversation

rayangler
Copy link
Collaborator

@rayangler rayangler commented Oct 15, 2020

[DOP-1604]
[Realms Staging /android/mongodb]

This PR introduces the inclusion of captions to code blocks. caption options are handled as strings. Code blocks now use showWindowChrome.

Copy link
Member

@sophstad sophstad left a comment

Choose a reason for hiding this comment

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

The A/C for this ticket definitely could have been more clear, but the goal is to show the caption in LeafyGreen's "chrome window" text, if it's specified. But so far so good—lemme know if I can clarify anything for you!

font-weight: bold;
`}
>
{caption}
Copy link
Member

Choose a reason for hiding this comment

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

Just flagging: once this is implemented, we'll probably need to use a ComponentFactory here to render the caption.

@@ -40,7 +40,7 @@ const getLanguage = lang => {
};

const Code = ({
nodeData: { copyable, emphasize_lines: emphasizeLines, lang, linenos, value },
nodeData: { copyable, caption, emphasize_lines: emphasizeLines, lang = 'none', linenos, value },
Copy link
Member

Choose a reason for hiding this comment

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

Did you encounter a bug without this default none for lang? I recently changed this component to handle that in getLanguage(), so wondering if you're seeing something weird!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, my mistake! I accidentally left lang = 'none' because of a merge conflict. I'll fix this

@@ -60,6 +60,17 @@ const Code = ({
margin: ${theme.size.default} 0;
`}
>
{/* This implementation of caption should change with DOP-1639 */}
{caption && (
Copy link
Member

@sophstad sophstad Oct 15, 2020

Choose a reason for hiding this comment

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

The ticket was definitely unclear about this, but we actually want to render the caption as the component's chromeTitle, if it exists!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, gotcha. Will change

@@ -72,6 +83,7 @@ const Code = ({
highlightLines={emphasizeLines}
language={getLanguage(lang)}
showLineNumbers={linenos}
showWindowChrome={true}
Copy link
Member

Choose a reason for hiding this comment

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

We only want this property to be true if a caption is specified.

@rayangler
Copy link
Collaborator Author

With the recent comment on DOP-1639, it seems like we do not need to worry about breaking the caption down into individual components. Captions are just strings now.

@rayangler rayangler merged commit 7671770 into mongodb:master Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants