Refactored `clippify.js` #1968

adamzap · 2025-02-28T22:55:23Z

Stopped using jQuery
Moved refactored code to djangoproject.js

The easiest way to test this for me was to add two code snippets to the bottom of djangoproject/templates/base.html:

<div class="highlight-default snippet">
  <div class="snippet-filename">polls/views.py</div>
  <div class="highlight">
    <pre><span></span><span class="kn">from</span> <span class="nn">django.shortcuts</span> <span class="k">import</span> <span class="n">get_object_or_404</span><span class="p">,</span> <span class="n">render</span>

<span class="k">def</span> <span class="nf">results</span><span class="p">(</span><span class="n">request</span><span class="p">,</span> <span class="n">question_id</span><span class="p">):</span>
    <span class="n">question</span> <span class="o">=</span> <span class="n">get_object_or_404</span><span class="p">(</span><span class="n">Question</span><span class="p">,</span> <span class="n">pk</span><span class="o">=</span><span class="n">question_id</span><span class="p">)</span>
    <span class="k">return</span> <span class="n">render</span><span class="p">(</span><span class="n">request</span><span class="p">,</span> <span class="s1">'polls/results.html'</span><span class="p">,</span> <span class="p">{</span><span class="s1">'question'</span><span class="p">:</span> <span class="n">question</span><span class="p">})</span>
    </pre>
  </div>
</div>

<div class="highlight-default snippet">
  <div class="snippet-filename">polls/views.py</div>
  <div class="highlight">
    <pre><span></span><span class="kn">from</span> <span class="nn">django.shortcuts</span> <span class="k">import</span> <span class="n">get_object_or_404</span><span class="p">,</span> <span class="n">render</span>

<span class="k">def</span> <span class="nf">results_2</span><span class="p">(</span><span class="n">request</span><span class="p">,</span> <span class="n">question_id</span><span class="p">):</span>
    <span class="n">question</span> <span class="o">=</span> <span class="n">get_object_or_404</span><span class="p">(</span><span class="n">Question</span><span class="p">,</span> <span class="n">pk</span><span class="o">=</span><span class="n">question_id</span><span class="p">)</span>
    <span class="k">return</span> <span class="n">render</span><span class="p">(</span><span class="n">request</span><span class="p">,</span> <span class="s1">'polls/results.html'</span><span class="p">,</span> <span class="p">{</span><span class="s1">'question'</span><span class="p">:</span> <span class="n">question</span><span class="p">})</span>
    </pre>
  </div>
</div>

- Stopped using jQuery - Moved refactored code to `djangoproject.js`

adamzap · 2025-02-28T22:56:53Z

djangoproject/static/js/djangoproject.js

+
+// Attach copy functionality to dynamically-created buttons
+document.querySelectorAll('.btn-clipboard').forEach(function (el) {
+  el.addEventListener('click', function () {


The code in this handler is largely the same as before (from #1809).

adamzap · 2025-02-28T22:57:31Z

djangoproject/static/js/djangoproject.js

+
+// Add copy buttons to code snippets
+(function () {
+  const button_el = document.createElement('span');


The old code also dynamically creates the buttons. It's just more verbose here without jQuery.

pauloxnet

LGTM

bmispelon

A rare PR from Adam that has more lines added than removed 😁

In all seriousness, this is a good PR and I'd be happy to see it merged. I had two minor suggestions (using innerText and innerHTML) a giant scope-creep comment about adding support for more complicated copying flows. I'm happy to be told "let's address this in another ticket", but I thought I'd mention it here in case you also think it's worth addressing.

Excellent PR as always (I especially appreciate your clear instructions on how to test your work, it makes reviewing so much easier 🏆 )

bmispelon · 2025-03-03T21:47:18Z

djangoproject/static/js/djangoproject.js

+    });
+
+    function on_success(el) {
+      success_el.appendChild(document.createTextNode('Copied!'));


I might be missing something, but why not simply use innerText here?

Suggested change

success_el.appendChild(document.createTextNode('Copied!'));

success_el.innerText = "Copied!";

Oh yes, thanks! Fixed in 9e5e21a.

bmispelon · 2025-03-03T21:48:52Z

djangoproject/static/js/djangoproject.js

+
+  const icon_el = document.createElement('i');
+
+  icon_el.classList.add('icon', 'icon-clipboard');
+
+  button_el.appendChild(icon_el);


Couldn't we use innerHTML here?

Suggested change

const icon_el = document.createElement('i');

icon_el.classList.add('icon', 'icon-clipboard');

button_el.appendChild(icon_el);

button_el.innerHTML = '<i class="icon-clipboard"></i>';

Yes, thank you! Fixed in c1f2744!

bmispelon · 2025-03-03T22:05:52Z

djangoproject/static/js/djangoproject.js

+      }, 5000);
+    }
+
+    const text = this.parentElement.nextElementSibling.textContent.trim();


This does works for both old-style snippet and the newer code-block-caption but I feel that it's too rigid.

In #1276 there seem to be two additional use-cases emerging:

copy from the split tabs with shell commands (unix / windows), where getting the text to be copied is a bit more involved

copy from a single <pre> wihout a caption/filename

Let me know if you think this is out of scope, but i think could be worth it if this new approach could support these uses-cases (at least theoretically, we probably don't want to implement them in this very PR).
A rough idea would be to have an array where each item has three elements:

A selector (like .snippet-filename or .code-block-caption)

A function that returns a DOM element where the copy button should be appended

A function that returns the text to be copied when the button is clicked.

Initially this array would have two items, but we could extend it with the other usecases I mentionned. What do you think?

I think something like what you have proposed here could be nice. I wonder how much control we have over the generation of these elements as it'd be nice to make them more consistent at the source. That may not be possible with the docs though.

I can add this to my list of things to evaluate. I'd want to see how bad the code is without the array approach first unless you feel strongly about starting there.

I agree that this effort should happen in another PR.

I think something like what you have proposed here could be nice. I wonder how much control we have over the generation of these elements as it'd be nice to make them more consistent at the source. That may not be possible with the docs though.

With snippet-filename, that's an old version of a custom sphinx plugin that used to be installed when building the Django docs (judging by the comments it got removed around the release of 2.0 which was a while back 👴🏻 ). The way the documentation works, those files are not getting rebuilt anymore, so modifying them is basically impossible.
As for code-block-caption, I believe that's a builtin sphinx feature, so changing the HTML there would involve writing a custom plugin. I don't believe that's worth the effort here.

Ok, good to know. Thanks for the context!

adamzap · 2025-03-03T22:29:42Z

@bmispelon Thank you for the suggestions on using innerText and innerHTML. I'm still getting used to the default DOM API for creating elements.

Refactored clippify.js

7f47165

- Stopped using jQuery - Moved refactored code to `djangoproject.js`

adamzap commented Feb 28, 2025

View reviewed changes

pauloxnet approved these changes Mar 2, 2025

View reviewed changes

bmispelon reviewed Mar 3, 2025

View reviewed changes

bmispelon mentioned this pull request Mar 3, 2025

Refactor SCSS and HTML templates for improved code snippet presentation #1929

Open

adamzap added 2 commits March 3, 2025 16:26

Use innerText where possible

9e5e21a

Use innerHTML for icon element

c1f2744

bmispelon merged commit d6d6ce7 into django:main Mar 4, 2025
4 checks passed

adamzap deleted the refactor-clippify-js branch March 4, 2025 19:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Refactored `clippify.js` #1968

Refactored `clippify.js` #1968

Uh oh!

adamzap commented Feb 28, 2025 •

edited

Loading

Uh oh!

adamzap Feb 28, 2025

Uh oh!

adamzap Feb 28, 2025

Uh oh!

pauloxnet left a comment

Uh oh!

bmispelon left a comment

Uh oh!

bmispelon Mar 3, 2025

Uh oh!

adamzap Mar 3, 2025

Uh oh!

bmispelon Mar 3, 2025

Uh oh!

adamzap Mar 3, 2025 •

edited

Loading

Uh oh!

bmispelon Mar 3, 2025

Uh oh!

adamzap Mar 3, 2025

Uh oh!

bmispelon Mar 4, 2025

Uh oh!

adamzap Mar 4, 2025

Uh oh!

adamzap commented Mar 3, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

	success_el.appendChild(document.createTextNode('Copied!'));
	success_el.innerText = "Copied!";

Uh oh!

Refactored clippify.js #1968

Refactored clippify.js #1968

Uh oh!

Conversation

adamzap commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pauloxnet left a comment

Choose a reason for hiding this comment

Uh oh!

bmispelon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamzap Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamzap commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Refactored `clippify.js` #1968

Refactored `clippify.js` #1968

adamzap commented Feb 28, 2025 •

edited

Loading

adamzap Mar 3, 2025 •

edited

Loading

adamzap commented Mar 3, 2025 •

edited

Loading