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(v2): add filename in CodeBlock #2346

Merged
merged 11 commits into from
Mar 25, 2020

Conversation

kohheepeace
Copy link
Contributor

Motivation

Add filename in CodeBlock which is mentioned in #2271

If you write below markdown like below, it will create filename in CodeBlock.

```js:src/pages/hello.js {2}
console.log('hogehoge');
```

Screenshot of output

[dark mode]
ss-of-code-filename-docusaurus-dark

[white mode]
ss-of-code-filename-docusaurus

I like this markdown syntax to create "filename", but it depends on docusaurus team.

```js:src/pages/hello.js {2}
console.log('hogehoge');
```

Have you read the Contributing Guidelines on pull requests?

Yes!

Test Plan

Add CodeBlock with this syntax in your doc.

```js:src/pages/hello.js {2}
console.log('hogehoge');
```

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 1, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Mar 1, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 13e024f

https://deploy-preview-2346--docusaurus-2.netlify.com

.fileName {
padding: 4px 12px;
font-weight: bold;
text-decoration: underline;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to separate these two blocks via the bottom border.

code title

code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a boderBottom: '1px solid'; one with

<pre className={classnames(className, styles.codeBlock)} style={style}>

ss-for-docusaurus-scroll-codeblock

The problem is

  1. I couldn't find easiest way to filename div width 100%.
  2. That's why boder-bottom is lacking.
  3. I found easy solution to set text-decoration: underline; and looks ok.

@@ -40,6 +40,7 @@ export default ({children, className: languageClassName, metastring}) => {

const target = useRef(null);
const button = useRef(null);
const fileName = languageClassName.split(':')[1];
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this feature more general? It seems to me that we need to look at this as a code title, not as just a code filename. For example, I want to add some text that has spaces in it, will it work?

```js:"My Code title" {2}
console.log('123');
```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree! And currently your example not working 😢

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add such an opportunity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure!

@lex111 lex111 changed the title feat: add filename in CodeBlock feat(v2): add filename in CodeBlock Mar 1, 2020
@kohheepeace
Copy link
Contributor Author

kohheepeace commented Mar 1, 2020

@lex111 Hi, I update code! Thanks for your review.

1. To use Regex to find title from markdown.

I'm not mature to write clean code to accomplish the below syntax 😢

```js:"Code title" {2}
console.log('hogehoge');
```

Current mdx api is not good for above syntax.
mdx-js/mdx#702

That's why I change markdown syntax like below. (order doesn't matter.)

```js {2} title="Code title"
console.log('hogehoge');
```

2. Change design style based on your advice

  1. I update code to accomplish your suggestion.
  2. And I forgot considering about "when code title is blank" in last commit.
  3. That's why code becomes a little bit messy.

[CodeBlock with title]
Screenshot (5)
Screenshot (4)

[CodeBlock without title]
Screenshot (6)

Thanks!

Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

@kohheepeace thanks for rework!

Remember to also duplicate these changes in live codeblock https://github.com/facebook/docusaurus/blob/a2fb0648340778428880e19e88a2aa84b1154329/packages/docusaurus-theme-live-codeblock/src/index.js

And as a demo add a code title in any code block to website/docs/ (for "package.json" here eg)

<button
ref={button}
type="button"
aria-label="Copy code to clipboard"
className={styles.copyButton}
className={classnames(styles.copyButton, {
[`${styles.copyButtonWithTitle}`]: codeBlockTitle,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just use [styles.copyButtonWithTitle]: codeBlockTitle,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh okay! Nice!

@kohheepeace
Copy link
Contributor Author

kohheepeace commented Mar 9, 2020

@lex111 I will re-PR by end of today with docs and liveCodeBlock! Thanks for your review!

@kohheepeace
Copy link
Contributor Author

@lex111 I updated code 👍

Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

Hi @kohheepeace sorry for the long answer, but we need to remove a fixed height for the code title, since on mobiles we observe the following:

Current Should be
image image

I think it is possible, as it was earlier, to move copy button to the pre element and position this button accordingly with respect to the pre element.

@kohheepeace
Copy link
Contributor Author

@lex111 thanks for taking time for review! I will fix this asap. sorry !

@kohheepeace
Copy link
Contributor Author

kohheepeace commented Mar 16, 2020

@lex111 I updated code.

move copy button to the pre element and position this button accordingly with respect to the pre element.

Just by doing 👆 leads to horizontal scroll bug like below. (It worked before because copy-button is absolute towards mdxCodeBlock but this doesn't work by adding code title block.)
error-gif-codeblock

That's why I also need to separate code block and title. 😢
(=> I needed to add a new container. Code become messy 😢 )

@lex111
Copy link
Contributor

lex111 commented Mar 16, 2020

@kohheepeace most likely yes, in this situation, an additional container is indispensable. Although perhaps @yangshun can tell an alternative solution.

In the meantime, let's just rename codeBlockWrapper to codeBlockContent.

And also make copy button appear when you hover over code title with this code:

.codeBlockTitle:hover + .codeBlockContent .copyButton,
.codeBlockContent:hover > .copyButton {
  visibility: visible;
  opacity: 1;
}

@kohheepeace
Copy link
Contributor Author

In the meantime, let's just rename codeBlockWrapper to codeBlockContent.

=> super agreed 👍

And also make copy button appear when you hover over code title with this code:

=>okay 👍

@kohheepeace
Copy link
Contributor Author

@lex111 @yangshun updated code👍

Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

Little docs improvements.

website/docs/markdown-features.mdx Outdated Show resolved Hide resolved
website/docs/markdown-features.mdx Outdated Show resolved Hide resolved
Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

👍

@yangshun please review.

@lex111 lex111 added this to the v2.0.0-alpha.49 milestone Mar 17, 2020
@lex111 lex111 added the pr: new feature This PR adds a new API or behavior. label Mar 21, 2020
@markerikson
Copy link

Would love to see this get in so we can use it in the Redux docs!

@lex111
Copy link
Contributor

lex111 commented Mar 23, 2020

@kohheepeace I updated the code blocks markup in master branch, so conflicts have appeared in your PR now, could you please resolve them?

@kohheepeace
Copy link
Contributor Author

@lex111 okay! Should I rebase or merge commit?

@lex111
Copy link
Contributor

lex111 commented Mar 23, 2020

@kohheepeace the usual (merge) commit will be enough.

@kohheepeace
Copy link
Contributor Author

kohheepeace commented Mar 23, 2020

@lex111 I merged master and fixed conflict related this PR 👍 #2442

=> Sorry 😢 It looks unrelated file changes is included. I will fix this.

@kohheepeace kohheepeace force-pushed the kohheepeace-codeblock-filename branch from 2a2e80a to 13e024f Compare March 23, 2020 13:58
@kohheepeace
Copy link
Contributor Author

@lex111 sorry for my poor git knowledge. I fixed conflict.

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

@yangshun yangshun merged commit 5e0d11d into facebook:master Mar 25, 2020
@markerikson
Copy link

yayyy! #2456 next? :)

@OlofHarrysson
Copy link

OlofHarrysson commented Apr 11, 2020

Hi, I'm having some problems getting this to work on my end. The title doesn't show up when I'm copying this code from the docs. https://v2.docusaurus.io/docs/markdown-features#code-title

```jsx title="src/components/HelloCodeTitle.js"
function HelloCodeTitle(props) {
  return <h1>Hello, {props.name}</h1>;
}
```

I'm on version 2.0.0-alpha.50. Not very experienced with web dev.

@lex111
Copy link
Contributor

lex111 commented Apr 11, 2020

Are you sure you're using the latest version (eg. try reinstall deps)? I see no other reason why it can't work. We need the repository so that we can reproduce this issue.

@OlofHarrysson
Copy link

Hi lex111, no I'm not sure. I tried following the upgrade instructions.

When you hinted that I might be on the wrong version, I checked my package.json file which I've already updated.

  "dependencies": {
    "@docusaurus/core": "^2.0.0-alpha.50",
    "@docusaurus/preset-classic": "^2.0.0-alpha.50",
    "@docusaurus/theme-live-codeblock": "^2.0.0-alpha.39",
    "@docusaurus/theme-search-algolia": "^2.0.0-alpha.32",

I changed so that all the versions were alpha.50, not just the core and preset-classic. That fixed the title issue I had for the code blocks, at least they show up now. But there is some issue with the styling, no rounded corner and can't read the copy button text when mouseover.

I'll push my code to my repo,
https://github.com/OlofHarrysson/anyfig if you want to have a look

I'd be willing to try to reinstall reps, how can I do that? yarn install? That gives me a few warning

@lex111
Copy link
Contributor

lex111 commented Apr 12, 2020

@OlofHarrysson we are aware of this issue and have already fixed it, so we will soon make a new release that will also include this fix.
By the way, you could just specify two deps (core and preset-classic), the rest you can remove (eg. https://github.com/facebook/docusaurus/blob/master/website/package.json#L12)

@OlofHarrysson
Copy link

@lex111 Ok great! I'll wait for the next release and work on my content in the meantime. Thanks for your help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants