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.

Sign up for GitHub

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

fix(theme-classic): inconsistent code block wrapping #7485

Merged
merged 6 commits into from
Jun 2, 2022
Merged

fix(theme-classic): inconsistent code block wrapping #7485

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d6db387
fix(theme-classic): inconsistent code block wrapping
dpang314 May 25, 2022
9eec193
refactor(theme-classic): abstract mutation observer
dpang314 May 25, 2022
e2864bc
fix: MutationObserver reference error
dpang314 May 26, 2022
062348e
Merge branch 'main' into code-wrap
slorber Jun 2, 2022
2eb5951
refactor
slorber Jun 2, 2022
df58da0
refactor
slorber Jun 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 32 additions & 2 deletions packages/docusaurus-theme-common/src/hooks/useCodeWordWrap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ export function useCodeWordWrap(): {
const [isEnabled, setIsEnabled] = useState(false);
const [isCodeScrollable, setIsCodeScrollable] = useState<boolean>(false);
const codeBlockRef = useRef<HTMLPreElement>(null);
const [mutationObserver, setMutationObserver] =
useState<MutationObserver | null>(null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be a state or a ref? We have usage of IntersectionObserver in core, and we use a ref there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We shouldn't need to re-render after IO is set, so ref is better 👍


const toggle = useCallback(() => {
const codeElement = codeBlockRef.current!.querySelector('code')!;
Expand All @@ -25,18 +27,28 @@ export function useCodeWordWrap(): {
codeElement.removeAttribute('style');
} else {
codeElement.style.whiteSpace = 'pre-wrap';
codeElement.style.overflowWrap = 'anywhere';
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The whiteSpace style will never break words apart, so for a single word that is long enough to overflow the code block, it would stay on one line and cause the scroll bar to remain.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 will add this back sorry

Copy link
Collaborator

Choose a reason for hiding this comment

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

#7551

}

setIsEnabled((value) => !value);
}, [codeBlockRef, isEnabled]);

const updateCodeIsScrollable = useCallback(() => {
const {scrollWidth, clientWidth} = codeBlockRef.current!;
// Allows code block to update scrollWidth and clientWidth after "hidden"
// attribute is removed
const hiddenAncestor = codeBlockRef.current?.closest('[hidden]');
if (hiddenAncestor && mutationObserver) {
mutationObserver.observe(hiddenAncestor, {
attributes: true,
attributeFilter: ['hidden'],
});
}
const isScrollable =
scrollWidth > clientWidth ||
codeBlockRef.current!.querySelector('code')!.hasAttribute('style');
setIsCodeScrollable(isScrollable);
}, [codeBlockRef]);
}, [codeBlockRef, mutationObserver]);

useEffect(() => {
updateCodeIsScrollable();
Expand All @@ -47,10 +59,28 @@ export function useCodeWordWrap(): {
passive: true,
});

if (!mutationObserver) {
setMutationObserver(
new MutationObserver((mutations) => {
mutations.forEach((mutation) => {
if (
mutation.type === 'attributes' &&
mutation.attributeName === 'hidden'
) {
updateCodeIsScrollable();
}
});
}),
);
}

Copy link
Collaborator

@slorber slorber May 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

it's a good practice in React to use one separate effect per concern (resize, observe mutations...) instead of trying to fit everything into a single effect

Also I suggest to create a reusable abstraction here to encapsulate the IO complexity.

For example: useMutationObserver() could be added to theme-common as it would likely be useful in other places: https://www.30secondsofcode.org/react/s/use-mutation-observer

note: we may want to take care of memorizing to avoid IO recreation:

And you can still extract a custom hook dedicated to this specific case.

In the end the component code could look as simple as: useHiddenAttributeMutationObserver(() => updateCodeIsScrollable());

Copy link
Contributor Author

@dpang314 dpang314 May 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for the help! I started implementing useMutationObserver() but had trouble passing in the target element, since it is constantly changing, and the mutation observer must call observe on the bew element each time it is changed. I followed the link, but instead of passing in a RefObject I pass in an an Element instead of a in order to trigger useEffect(). Is this fine?

I also have another implementation that does not need to store any additional state. It does cause errors with the linter (@typescript-eslint/no-use-before-define), but it builds and works properly. However, it is less reusable and not as well encapsulated. Is it better to go with this method?

Inside useMutationObserver:

return {
    setAncestor: (ancestor: Element | null | undefined): void => {
      if (ancestor) {
        mutationObserver.current.observe(ancestor, options);
      }
    }
  }

Inside useCodeWordWrap:

const observer = useMutationObserver((mutations) => {
    mutations.forEach((mutation) => {
      if (
        mutation.type === 'attributes' &&
        mutation.attributeName === 'hidden'
      ) {
        updateCodeIsScrollable();
      }
    });
  });

Inside updateCodeisScrollable:

observer.setAncestor(codeBlockRef.current?.closest('[hidden]'));

return () => {
window.removeEventListener('resize', updateCodeIsScrollable);
if (mutationObserver) {
mutationObserver.disconnect();
}
};
}, [updateCodeIsScrollable]);
}, [updateCodeIsScrollable, mutationObserver]);

return {codeBlockRef, isEnabled, isCodeScrollable, toggle};
}
59 changes: 59 additions & 0 deletions website/_dogfooding/_pages tests/code-block-tests.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import CodeBlock from '@theme/CodeBlock';
import BrowserWindow from '@site/src/components/BrowserWindow';
import Tabs from '@theme/Tabs';
import TabItem from '@theme/TabItem';

# Code block tests

Expand Down Expand Up @@ -190,3 +192,60 @@ function PageLayout(props) {
);
}
```

## Code block wrapping tests

[// spell-checker:disable]: #
Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh, I didn't know this is a thing 🤔


```bash
mkdir this_is_a_loooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong_string_to_test_code_block_wrapping
echo "this is a long string made up of many separate words that should be broken between words when possible"
curl https://docusaurus.io/tests/pages/code-block-tests
```

<Tabs>

<TabItem value="short-tab-1" label="Short tab">

```bash
echo "hi"
```

</TabItem>
<TabItem value="long-tab" label="Long tab">

```bash
mkdir this_will_test_whether_a_long_string_that_is_initially_hidden_will_have_the_option_to_wrap_when_made_visible
```

</TabItem>

<TabItem value="short-tab-2" label="Short tab">

```bash
rm short_initially_hidden_string
```

</TabItem>
</Tabs>

<Tabs>

<TabItem value="long-tab" label="Long tab">

```bash
echo medium_length_string_will_have_the_option_to_wrap_after_window_resized_while_it_is_hidden
```

</TabItem>

<TabItem value="short-tab" label="Short tab">

```bash
echo "short_initially_hidden_string"
```

</TabItem>
</Tabs>

[// spell-checker:enable]: #