-
-
Notifications
You must be signed in to change notification settings - Fork 31.2k
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
Fixed #34846 -- Added copy button to code snippets on the documentation #18247
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Alexander Lötvall <[email protected]> Co-authored-by: Saarthak Maini <[email protected]> Co-authored-by: Venkata Bhaskar <[email protected]>
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.
Hello! Thank you for your contribution 💪
As it's your first contribution be sure to check out the patch review checklist.
If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!
If you have any design or process questions then you can ask in the Django forum.
Welcome aboard ⛵️!
@@ -2,3 +2,4 @@ pyenchant | |||
Sphinx>=4.5.0 | |||
sphinxcontrib-spelling | |||
blacken-docs | |||
sphinx-copybutton |
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.
Thanks for the PR!
Is there a way to achieve this without adding this as a new dependency?
I'm slightly concerned about the maintenance status of the underlying js library. What do you think?
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.
Thanks for the response, and yeah most probably there is, I will look into it.
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.
What’s your concern @smithdc1? Anecdotally we’ve been using this same dependency for the Wagtail docs for two years now, haven’t had a problem yet.
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.
@arthurvasconcelos did you see the comment on the last PR that this conflicts with JavaScript in djangoproject.com and perhaps should be solved there rather than adding a dependency here?
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.
Purely from an accessibility perspective, I’ve reviewed the sphinx_copybutton
implementation and am very happy with how it works.
I've been trying to see how this change would actually look like once deployed to docs.djangoproject.com and it has been more challenging than I had thought (the djangoproject website does not use I hope to be able to get to the bottom of this in the upcoming week, but please ping me if I forget 😁 |
Trac ticket number
Done during Django Conference Europe 2024 sprint 🎉
ticket-34846
Branch description
This solution will use the sphinx framework provided copy-button to copy the whole code. On the original PR(#16342) I found this comment #16342 (comment) which I struggle to understand why this solution would conflict. When checking https://github.com/django/djangoproject.com/blob/main/djangoproject/static/js/mod/clippify.js I found them to be different projects using different frameworks, could some extra context be provided on the possible conflict?
Light:
![image](https://private-user-images.githubusercontent.com/1286768/337875419-d6b9cba4-fcd6-4cbf-b61d-2bad8150e244.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTkzODI4MjMsIm5iZiI6MTcxOTM4MjUyMywicGF0aCI6Ii8xMjg2NzY4LzMzNzg3NTQxOS1kNmI5Y2JhNC1mY2Q2LTRjYmYtYjYxZC0yYmFkODE1MGUyNDQucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDYyNiUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA2MjZUMDYxNTIzWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9Y2UyYTdmMTQ1MmI4OGNhYjEzZDgyNGMzM2FkYzE5NWI2YmEzMTc1MTRjZWU5M2YyOGE3Y2Q3MDBjMTQ5MzI2NCZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.Wl3ZO_ljcNzx11a_E2-dB1z-P6XLg9Lpz2qoE0BeJWc)
Dark:
I couldn't find how to switch to dark mode.
Checklist
main
branch.