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

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

Merged
merged 6 commits into from
Jun 2, 2022

Conversation

dpang314
Copy link
Contributor

@dpang314 dpang314 commented May 25, 2022

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

Resolves #7449

Addresses the following issues:

  • Wrap code button not breaking apart long words
  • Wrap code button not appearing when switching to initially hidden tab components

Also fixes one other issue that can be replicated as follows

  1. Start on a tab component with the window wide enough that it does not have a wrap code button
  2. Switch to another tab component
  3. While still on the second tab, make the window narrow enough that the original tab would become scrollable
  4. Switch back to the original tab. Even though it is scrollable, it will not have a code wrap button

The issues with hidden tabs were caused by elements with an ancestor with the "hidden" attribute that caused their clientWidth and scrollWidth to be 0, so they could not properly determine whether the code block was scrollable. The new code looks for ancestors with the "hidden" attribute and watches for when they are no have the "hidden" attribute. At that point, the clientWidth and scrollWidth are accurate and it can determine whether the code block is scrollable.

Test Plan

Tests added to bottom of dogfooding code-block-tests.mdx

Before

before.mp4

After

after.mp4

Test links

Deploy preview: https://deploy-preview-7485--docusaurus-2.netlify.app/tests/pages/code-block-tests#code-block-wrapping-tests
@alexfornuto's example with bugs resolved: https://rad-bienenstitch-2b77e5.netlify.app/tests/pages/long-code-block-tests

Related issues/PRs

Fixes bugs in #7036
Resolves #7449

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 25, 2022
@netlify
Copy link

netlify bot commented May 25, 2022

[V2]

Name Link
🔨 Latest commit df58da0
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/6298b59249e1f90009c8f881
😎 Deploy Preview https://deploy-preview-7485--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 May 25, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 69 🟢 100 🟢 100 🟢 100 🟢 90 Report
/docs/installation 🟠 78 🟢 99 🟢 100 🟢 100 🟢 90 Report

@Josh-Cena Josh-Cena added the pr: bug fix This PR fixes a bug in a past release. label May 25, 2022

## 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 🤔

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

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 like a good solution thanks 👍

I'd like the complexity of IO to be more encapsulated, let me know if you need help

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

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

Comment on lines 62 to 76
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

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

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]'));

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.

👍

Did some cleanup but it looks to work fine

const mutationObserver = useRef<MutationObserver | undefined>(
ExecutionEnvironment.canUseDOM ? new MutationObserver(callback) : undefined,
);
const memoOptions = useMemo(() => options, [options]);
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 useless memo

@@ -25,19 +52,25 @@ 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.

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: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code Block Wrapping Behavior is Inconsistent.
4 participants