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(theme-classic): toggle code wrap button #7036

Merged
merged 13 commits into from
Apr 22, 2022
Merged

feat(theme-classic): toggle code wrap button #7036

merged 13 commits into from
Apr 22, 2022

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Mar 28, 2022

Motivation

Code blocks with long lines sometimes inconvenient to read, especially on mobile devices. But what if next to the copy code button, there was a button to toggle line wrapping? I mean that button would show up only when needed (there are long lines in any code block which cause horizontal scrolling).

I'm still not sure about the need for this feature, but decided to implement it to demonstrate it action.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

ezgif com-gif-maker (4)

Code for testing:

```js
function PageLayout(props) {
  // highlight-next-line
  return <Layout title="My awesome Docusaurus page" description="Just one more awesome page on my Docusaurus ite">;
}
```

https://deploy-preview-7036--docusaurus-2.netlify.app/tests/pages/code-block-tests

Related PRs

@lex111 lex111 added the pr: new feature This PR adds a new API or behavior. label Mar 28, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 28, 2022
@netlify
Copy link

netlify bot commented Mar 28, 2022

[V2]

Name Link
🔨 Latest commit 0464aa9
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/6262a16fa0abae00094480d9
😎 Deploy Preview https://deploy-preview-7036--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Mar 28, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 63
🟢 Accessibility 100
🟢 Best practices 92
🟢 SEO 100
🟢 PWA 90

Lighthouse ran on https://deploy-preview-7036--docusaurus-2.netlify.app/

@github-actions
Copy link

github-actions bot commented Mar 28, 2022

Size Change: +194 B (0%)

Total Size: 802 kB

Filename Size Change
website/build/assets/css/styles.********.css 107 kB +194 B (0%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 50 kB
website/build/assets/js/main.********.js 606 kB
website/build/index.html 38.8 kB

compressed-size-action

@Josh-Cena
Copy link
Collaborator

Josh-Cena commented Mar 28, 2022

I'm not sure about the value of this feature either 🤷‍♂️ Do we have examples of existing sites with this feature?

There's a Canny request for this: https://docusaurus.canny.io/feature-requests/p/toggle-line-wrap-in-code-blocks And I did vote for it long ago, but it doesn't seem to have any significant traction, and I also don't see huge value in having it OOTB

@lex111
Copy link
Contributor Author

lex111 commented Mar 28, 2022

This feature is more likely to be useful for mobiles users, because on small screens code blocks will usually be scrollable. So let's wait for feedback from @slorber then.
And no, I can't think of any docs sites that have this feature.

@Dannyholt
Copy link

I'd like to see this added as we include a lot of WordPress hook examples and so, wrapping the code would make this easier for a non-technical users and as @lex111 mentioned, would help on mobile devices. :)

@Josh-Cena
Copy link
Collaborator

I see. The mobile use-case makes a lot of sense.

@Josh-Cena Josh-Cena added the status: awaiting review This PR is ready for review, will be merged after maintainers' approval label Mar 30, 2022
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Looks interesting, willing to add this feature 👍

Some implementation details worth reworking

Comment on lines 59 to 74
.codeButton {
--docusaurus-code-button-size: 2rem;
position: absolute;
right: calc(var(--ifm-pre-padding) / 2);
top: calc(var(--ifm-pre-padding) / 2);
width: var(--docusaurus-code-button-size);
height: var(--docusaurus-code-button-size);
display: flex;
justify-content: center;
align-items: center;
background: inherit;
border: 1px solid var(--ifm-color-emphasis-300);
border-radius: var(--ifm-global-radius);
transition: opacity 200ms ease-in-out;
opacity: 0;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should try to separate this CSS in 2 parts:

  • the "inner" of a code button (can be extracted and moved to a separate CodeButton component)
  • the rules that are responsible to position the button inside the parent component

Comment on lines 22 to 51
setIsEnabled((value) => !value);

const codeElement = codeBlockRef.current!.querySelector('code');
codeElement?.classList.toggle(styles.codeWithWordWrap!);
}, [codeBlockRef]);
const updateCodeIsScrollable = useCallback(() => {
const {scrollWidth, clientWidth} = codeBlockRef.current!;
setIsCodeScrollable(scrollWidth > clientWidth);
}, [codeBlockRef]);
const title = translate({
id: 'theme.CodeBlock.wordWrapToggle',
message: 'Toggle word wrap',
description:
'The title attribute for toggle word wrapping button of code block lines',
});

useEffect(() => {
updateCodeIsScrollable();

window.addEventListener('resize', updateCodeIsScrollable);

return () => {
window.removeEventListener('resize', updateCodeIsScrollable);
};
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

if (!isCodeScrollable) {
return null;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

all this code is quite technical and shouldn't be there

The button should preferably always render a button, and users swizzling it should be able to change its display/icon without having to manage this technical code.

I don't think the button should either handle a ref of parent code block. It's the parent that should decide if the toggle button should be displayed, not the button itself.

I'd suggest moving this complexity to a custom hook in theme-common, and using the hook inside <CodeBlock> instead <WordWrapButton>

Comment on lines 146 to 149
<WordWrapButton
className={styles.codeButton}
codeBlockRef={codeBlockRef}
/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

What I have in mind:

Suggested change
<WordWrapButton
className={styles.codeButton}
codeBlockRef={codeBlockRef}
/>
{(wordWrap.enabled || wordWrap.codeScrollable) && (
<WordWrapButton
className={styles.codeButton}
onClick={() => wordWrap.toggle()}
/>
)}

// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

if (!isCodeScrollable) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Little issue:

  • mobile
  • enable word wrap
  • resize

Now isCodeScrollable = false and the button disappears => no way to turn word wrap off

image

*/

.wordWrapButton {
right: calc(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like the button to not decide where it will be rendered: that's the responsibility of the parent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like it either, I was thinking of grouping these buttons into one new div container, but it's not that easy because we would have to somehow get background of the current Prism color theme (need it for button background, not its container). Therefore inherit value won't work here, so maybe we dynamically set two new CSS vars, like as --prism-background and --prism-color? Will that be acceptable solution?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this works? usePrismTheme().plain.backgroundColor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, in the case of button container, we need to set the style attribute for each button inside to set the proper background. I think it's kind of redundant overhead, which we can avoid by dynamically creating two CSS vars that also can re-use in other places where the style attribute is currently used.

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lex111 FWIW, Prism also inlines their styles, so it won't be too harmful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just wouldn't want to overuse it, especially if there's a pretty appropriate alternative solution. Beside that, using inline styles would require creating one specific prop style for each button component, which can also be avoided.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this rule still necessary?

Looks to me it's not useful anymore => removing it and things keep working?

@lex111 lex111 force-pushed the lex111/code-wrap branch from ebb2d9e to 97e6922 Compare April 16, 2022 08:12
Copy link
Contributor Author

@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.

Ready for re-review.

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

👍

Fixed a minor edge case in 5a14be5

Minor cleanups can probably be done, I think there's some unused CSS

Wonder if we could share more code between the 2 buttons, but that doesn't seem critical to merge

*/

.wordWrapButton {
right: calc(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this rule still necessary?

Looks to me it's not useful anymore => removing it and things keep working?

}

.wordWrapButtonIcon {
width: 1.2rem;
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we want to use the same size for all buttongroup icons? and introduce anv env variable to customize?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't work that way, the proportions of both icons are different, so if we use the same sizing for them, the wordWrap button will look smaller than the copyButton icon:

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

we can keep it this way for now, but I guess the wordWrap svg could also be cropped somehow to take full space

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

LGTM thanks 👍

Will do the minor change and merge

import {useState, useCallback, useEffect, useRef} from 'react';

export function useCodeWordWrap(): {
readonly codeBlockRef: (node: HTMLPreElement | null) => void;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is not needed to use a function here you can return the React ref and assign it directly (like it was before, but instead it's created in the custom hook instead of the component

@slorber slorber merged commit 4e4aa6a into main Apr 22, 2022
@slorber slorber deleted the lex111/code-wrap branch April 22, 2022 12:50
@Josh-Cena Josh-Cena removed the status: awaiting review This PR is ready for review, will be merged after maintainers' approval label Apr 22, 2022
@alexfornuto
Copy link

It looks like this was merged without being documented. Because it's not working in my testing, I'm looking for (and not finding) a way to disable this feature.

@Josh-Cena
Copy link
Collaborator

@alexfornuto This is not an opt-in feature and there's no option for this. If you want to disable this, you have to swizzle CodeBlock and remove the relevant code.

What's not working for you?

@alexfornuto
Copy link

@Josh-Cena thanks for the reply. Example preview here shows the following example, two code blocks, each in a tab:

<Tabs>

<TabItem value="yum" label="Yum">

```abnf title="/etc/yum.repos.d/pomerium-pomerium.repo"
[pomerium-pomerium]
name=pomerium-pomerium
baseurl=https://dl.cloudsmith.io/public/pomerium/pomerium/rpm/el/$releasever/$basearch
repo_gpgcheck=1
enabled=1
gpgkey=https://dl.cloudsmith.io/public/pomerium/pomerium/gpg.6E388440B94E1407.key
gpgcheck=1
sslverify=1
pkg_gpgcheck=1
```

</TabItem>
<TabItem value="deb" label="Deb">

```bash
curl -1sLf 'https://dl.cloudsmith.io/public/pomerium/pomerium/gpg.6E388440B94E1407.key' | apt-key add -
echo "deb https://dl.cloudsmith.io/public/pomerium/pomerium/deb/debian buster main" > /etc/apt/sources.list.d/pomerium-pomerium.list
```

</TabItem>
</Tabs>

While other code blocks on the linked page display and use the wrap button, these do not behave as expected:

  • In the first tab the button is present but not functioning.
  • In the second tab the button is not present, despite the code block overflowing.

This example is using 2.0.0-beta.20

@Josh-Cena
Copy link
Collaborator

That seems like a bug. Please open an issue.

@magician11
Copy link

is there a way to toggle it on by default? I'm using it to show paragraphs of text..

@Josh-Cena
Copy link
Collaborator

@magician11 #7875 is the closest. If you think that won't satisfy your use case please submit a new issue instead of posting under an old PR.

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