Fix the visible loading delay of the copy page button #643
Conversation
Deploying configcat-docs with
|
| Latest commit: |
f6dbcb5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c131a1bc.docs-xmy.pages.dev |
| Branch Preview URL: | https://fix-copy-button-loading.docs-xmy.pages.dev |
codedbychavez
changed the title
codedbychavez
changed the title
regardless of the synthetic header.
|
🤖 Hi @codedbychavez, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request effectively addresses the loading delay of the copy page button by removing the client-side plugin and integrating the button directly into the Docusaurus theme. The code is well-refactored into TypeScript and seems to solve the primary issue.
🔍 General Feedback
- The main point of feedback is regarding the new placement of the copy button. It now appears above the page title, which differs from the previous design where it was next to the title. I've added a specific comment with a suggestion on how to adjust this if the change was unintentional.
- The overall approach of swizzling the
DocItem/Contentcomponent is a good solution for the problem described.
There was a problem hiding this comment.
This is an additional review with further feedback.
🔍 General Feedback
- I've identified a potential UI issue with the alignment of the dropdown menu on wider screens. The logic from the previous implementation that handled desktop alignment seems to have been removed, which could lead to the dropdown appearing off-screen. A suggestion to restore this responsive behavior has been added as an inline comment.
|
🤖 Hi @codedbychavez, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
📋 Review Summary
This pull request refactors the CopyPageButton from a Docusaurus plugin into a standalone React component, converting it to TypeScript and improving its integration. The changes are generally well-executed and improve the codebase.
🔍 General Feedback
- The move from a JavaScript-based plugin to a TypeScript React component is a great improvement for type safety and maintainability.
- The new integration using Docusaurus theme swizzling is cleaner than the previous approach that used global CSS selectors.
- I've identified one critical issue regarding the application of CSS module styles which will prevent the header from rendering correctly. Please see the inline comment for details.
…th a single solution.
|
🤖 Hi @codedbychavez, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
📋 Review Summary
This pull request effectively resolves the visible loading delay of the copy page button by replacing the previous Docusaurus plugin implementation with a swizzled Heading component. The new approach renders the button server-side along with the h1 title, which is a much better solution.
🔍 General Feedback
- The refactoring is well-executed. The new
CopyPageButton.tsxcomponent is robust and well-written. - The changes to the markdown files are appropriate and improve document structure.
- The solution is clean and directly addresses the root cause of the issue.
Great work on this!
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
🤖 Hi @codedbychavez, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
This pull request refactors the "Copy Page" button feature by removing the old plugin-based approach and integrating it directly into the theme by swizzling the Heading component. This is a good approach to fix the loading delay issue.
The overall code quality is good, with a thorough implementation of the content extraction and markdown conversion logic. However, there is a critical syntax issue in the new Heading component that will break the build, and a potential improvement for the CopyPageButton component.
🔍 General Feedback
- The refactoring to a swizzled component is a smart solution for the reported problem.
- The new
CopyPageButton.tsxis well-structured and handles many edge cases for content and code extraction. - Please address the critical issue in
website/src/theme/Heading/index.tsxto ensure the site can build successfully.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
🤖 Hi @codedbychavez, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
📋 Review Summary
This PR is a solid improvement to fix the loading delay of the copy button. Swizzling the Heading component is a clever approach. The refactoring of the CopyPageButton component into TypeScript is also a great enhancement, making it much cleaner.
🔍 General Feedback
- There is a critical syntax error in the new
Heading/index.tsxcomponent that needs to be fixed. - I've left a couple of minor suggestions for further cleanup and consistency, but they are not blocking.
Overall, great work on this! Just the one fix needed before it can be merged.
…nto fix-copy-button-loading
|
🤖 Hi @codedbychavez, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
📋 Review Summary
This pull request refactors the "copy page" button to address a loading delay. The approach of swizzling the Heading component is a good solution. The new CopyPageButton component is well-structured.
🔍 General Feedback
- I've identified a critical syntax error in the new
Headingcomponent that will prevent the site from building. Please see the inline comment for details and a suggested fix. - I've also left a suggestion on the
CopyPageButtonto consider using React Portals for the dropdown menu to improve its robustness against CSS stacking context issues.
Overall, this is a good improvement, and after fixing the critical issue, it should be ready.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
🤖 Hi @codedbychavez, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
📋 Review Summary
This pull request aims to fix a visible loading delay for the "copy page" button by integrating it directly into the Heading component via Docusaurus theme swizzling. The overall approach is solid and the refactoring from a plugin to a themed component is a good improvement for performance.
🔍 General Feedback
- The move to a swizzled component is a clever solution to the problem of injection delay.
- The new
CopyPageButton.tsxcomponent is well-written in TypeScript, with good use of hooks and clear structure. - I've found a critical syntax error in
website/src/theme/Heading/index.tsxthat will break the build, along with a minor code comment duplication. Please see the inline comments for details.
|
🤖 Hi @codedbychavez, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
📋 Review Summary
This pull request effectively resolves the loading delay of the copy page button by swizzling the Heading component and integrating the button directly. This is a solid architectural change that improves user experience. The code is well-refactored into a new TypeScript component, and the removal of the old plugin structure is clean.
🔍 General Feedback
- The refactoring of the copy button from a client-side injected plugin to a component directly integrated within the swizzled
Headingis a great improvement for performance and maintainability. - The new
CopyPageButton.tsxcomponent is well-structured and makes good use of React hooks likeuseCallbackanduseMemo. - The removal of redundant markdown titles in
feature-flag-evaluation.mdxandtargeting-rule-overview.mdxis a good content cleanup.
|
🤖 Hi @codedbychavez, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
📋 Review Summary
This pull request effectively resolves the loading delay of the copy page button by swizzling the Docusaurus Heading component. The new approach of integrating the button directly into the theme is a significant improvement over the previous client-side plugin. The refactoring to TypeScript and the overall code quality are excellent.
🔍 General Feedback
- The move from a Docusaurus plugin to a swizzled theme component is a great architectural choice for solving the initial problem.
- The new
CopyPageButton.tsxcomponent is well-structured and more maintainable. - The markdown file changes correctly remove redundant H1 headings.
|
🤖 Hi @codedbychavez, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
📋 Review Summary
This pull request effectively resolves the loading delay of the copy page button by swizzling the Docusaurus Heading component and integrating the button directly. The approach is sound and the implementation is clean.
🔍 General Feedback
- The refactoring from a Docusaurus plugin to a swizzled component is a good architectural choice for this specific problem.
- The new
CopyPageButtoncomponent is well-written in TypeScript, which improves type safety and maintainability. - The styling changes are appropriate and correctly handle both desktop and mobile views.
|
🤖 Hi @codedbychavez, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
📋 Review Summary
This pull request effectively resolves the loading delay issue for the "copy page" button by integrating it directly into the Heading component via Docusaurus theme swizzling. The implementation is clean, modern, and a significant improvement over the previous plugin-based approach.
🔍 General Feedback
- Positive: The conversion of
CopyPageButton.jsto TypeScript is a great enhancement, improving type safety and maintainability. - Positive: The code is well-refactored, making good use of React hooks like
useCallbackanduseMemo. - Suggestion: The removal of redundant H1 headings in the markdown files is a good cleanup. I've left one minor suggestion regarding a title change in one of the documents.
- Suggestion: I've also left a comment about potentially making the actions in the
CopyPageButtonconfigurable via props to maintain flexibility.
Overall, this is a high-quality contribution that improves user experience and the underlying codebase. Well done!
|
🤖 Hi @codedbychavez, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
This pull request effectively resolves the loading delay for the "copy page" button by integrating it directly into a swizzled Docusaurus Heading component. The approach is sound, and the conversion of the component to TypeScript and SCSS is a great improvement.
🔍 General Feedback
- The removal of the Docusaurus plugin in favor of theme component swizzling is a solid architectural change that directly addresses the original problem.
- I've added a couple of inline comments suggesting an improvement to a new validation script to avoid false positives and pointing out a minor feature regression regarding the configuration of the copy button's actions.
- Overall, the changes are of high quality and improve user experience.
|
Feel free to have a look. I've also updated the PR's description. Thanks for the feedback 💯 |
Describe the purpose of your pull request
This PR fixes the visible loading delay of the copy page button in which the button appears ~1 second after the page is loaded. This issue was reported to me after some QA checks from rest of the content team.
During research and brainstorming, I found that if the button element can be placed next to the H1 element in the source code, then the need for injecting it after the DOM is built would solve the visible delay.
This was accomplish by swizzling just the
@docusaurus/theme-classicHeadingcomponent, so the copy page button can be included next to the H1 heading of every page.Related issues (only if applicable)
n/a
How to test? (only if applicable)
Preview link: https://fix-copy-button-loading.docs-xmy.pages.dev/docs/advanced/caching/
What part of the application was affected by the changes?
What should be tested?
Requirement checklist