-
Notifications
You must be signed in to change notification settings - Fork 45
[FTF-431] fix: prefetch page Markdown content for better cross-browser support #3023
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
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
2d70eba to
78a20d7
Compare
78a20d7 to
9732493
Compare
9732493 to
b08d4d6
Compare
b08d4d6 to
199a1b5
Compare
95c5d63 to
683b3e2
Compare
683b3e2 to
80d102d
Compare
d611d88 to
b37a534
Compare
b37a534 to
ab2a059
Compare
dedc7d7 to
7853dc0
Compare
7853dc0 to
7be63c4
Compare
| try { | ||
| const response = await fetch(`${location.pathname}.md`); | ||
| useEffect(() => { | ||
| const abortController = new AbortController(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets us abort the fetch if dismounted, was playing with the tests a bit (and is a positive step)
| </Tooltip.Portal> | ||
| </Tooltip.Root> | ||
| </div> | ||
| {markdownContent && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made the choice to conditionally hide the markdown buttons if there's no content, since we now grab it ahead-of-time
2677f02 to
4b5e29e
Compare
f983fe6 to
1d8b29a
Compare
|
@kennethkalmer Taken another swing at this (PR updated and puffed out a bit with tests), was an interesting problem. Sorry for the delay on it, the tests were a nightmare. The main difference is that using Given the tiny payload it seems like a lesser evil (and also allows a runtime check to see if we should render the markdown buttons at all). Ready for a look at a point of convenience. |
kennethkalmer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jamiehenson thanks, this is a good compromise for the moment and a fantastic learning! I think in the new year we could enhance it with a bit of "feature detection" and then only prefetch the markdown if async clipboard writing is not supported. But for now this creates equal access for everyone, thank you!
"Copy as markdown" uses the modern
navigatorAPI to copy contents to the clipboard. This works poorly with asynchronous operations (i.e. in this case where we fetched markdown content and then copied it to clipboard) as it breaks the terms of what is considered a user interaction - and Safari is the most strict on this.The solution here is suboptimal, but save for programatically creating a text area and then having the user click it (a previous approach), this PR makes the operation synchronous by fetching the markdown copy on load and then copying it to the clipboard without
async. Naturally this is wasteful if only 1% of people click the markdown copy button, but the payload is very small and it allows for a smoother copy experience. If this is unsatisfactory my suggestion would be to drop the copy and just have the view link.Tests have been added for
PageHeaderto represent this.Also includes a very quick fix for the LeftSidebar overflow property (was getting horizontal scrolls on the bar)
To test: go here in Chrome and Safari and copying should work gracefully as elsewhere.