Feature/3683 url generator add copy button

[the-r3aper7]

Fixes #3683 Part B

After clicking Copy button

When no clipboard is defined

Please check the following:

• Do the tests still pass?¹
• Does the code comply with the style guide?
  • Run make lint from the Wagtail root.
• For Python changes: Have you added tests to cover the new/fixed behaviour?
• For front-end changes: Did you test on all of Wagtail’s supported environments?²
  • Please list the exact browser and operating system versions you tested:
  • Please list which assistive technologies ³ you tested:
• For new features: Has the documentation been updated accordingly?

Please describe additional details for testing this change.

Footnotes

1. Development Testing ↩

2. Browser and device support ↩

3. Accessibility Target ↩

[squash-labs]

Manage this branch in Squash

Test this branch here: https://the-r3aper7feature3683-url-gen-dx4wa.squash.io

[lb-]

Wow @the-r3aper7 - this is awesome.

Thanks for understanding the brief and working through + getting feedback on Slack.

I have done a review locally and it looks like the message is being duplicated each copy but then gets removed after some time so some tweaks may be needed.

I have also gone through the code and put a bunch of feedback, do not be overwhelmed, just work through comments one by one. Once you feel it's resolved and have pushed up changes just resolve the comment.

Finally, you may want to start working on a unit test suite for this new Clipboard controller.

I would suggest you look at client/src/controllers/ActionController.test.js as a bit of a reference for setting up a unit test.

When you feel it's ready for another round of feedback, please just add a message on the PR.

Thank you again for taking the time to contribute. I am super excited about seeing this land in core, it's not only a versatile feature (ability to copy text) but also makes this page much more useful for Wagtail editors.

[lb-]

Let's move this to alpha order - so it's easier to navigate through this file.

Just under bulk I think.

  { controllerConstructor: BulkController, identifier: 'w-bulk' },
++  { controllerConstructor: ClipboardController, identifier: 'w-clipboard' },

[lb-]

Can this import please be moved to alpha order.

import { BulkController } from './BulkController';
++import { ClipboardController } from './ClipboardController';
import { CloneController } from './CloneController';

[lb-]

Suggested change

	this.dispatch('start');
	const event = this.dispatch('copy');
	if (event.defaultPrevented) return;

Let's rename this as copy, it's probably a bit nicer than start.

Additionally with a one line defaultPrevented check we can enable a much more flexible usage of this in the future.

[lb-]

It's probably more common to put the rejection handler inside catch, unless I am missing something specific we need to do here.
e.g.

   navigator.clipboard.writeText(value).then(
      () => this.showSuccess(),
  ).catch(() => this.showError())

[lb-]

I would probably recommend a simpler approach, where we avoid the extra methods for success/error, bringing in the suggestions from above could look something like this.

Wrapping the initial check for navigator.clipboard in a promise means we can use a Promise resolve/reject all the way through.

The dispatching events will happen in one place, then/catch which means no need to abstract these out too early.

  copy() {
    const value = this.valueTarget.value;
    if (!value) return;

    const event = this.dispatch('copy');
    if (event.defaultPrevented) return;

    new Promise((resolve, reject) => {
      if (navigator.clipboard) {
        navigator.clipboard.writeText(value).then(resolve, reject);
      } else {
        reject();
      }
    })
      .then(() => this.dispatch('copied', { detail: { type: 'success' } }))
      .catch(() => this.dispatch('error', { detail: { type: 'error' } }));
  }

[the-r3aper7]

i have resolved the stacking issue. and updated the PR description

[lb-]

@the-r3aper7 just wanted to say thank you so much for your efforts here.

I have reviewed and this mostly works, I understand what you were trying to do with the clear delay and realised that the current clone controller does not support 'clearing after a time' but that value is more for how the classes work.

I have taken the liberty to add this to your PR as a feature and will merge in shortly.

I have also adjusted the classes a bit to use pure tailwind classes for the positioning of the note and fixed up the accessibility for this.

I hope this is OK, it was super close but just needed a bit more polish to get over the line.

I will push to your branch shortly and get this merged in (assuming the CI passes).

---

Screenshot of VoiceOver reading the label when it appears (when copied).

[lb-]

@the-r3aper7 I have added you to the contributors list as Sai Srikar Dumpeti. Let me know if you wanted a different name listed.

Well done on your first contribution - and for it being a really epic one. This is such a great functionality to have in core and a marked improvement for interacting with this page.

a57c63d

[the-r3aper7]

@the-r3aper7 I have added you to the contributors list as Sai Srikar Dumpeti. Let me know if you wanted a different name listed.

Well done on your first contribution - and for it being a really epic one. This is such a great functionality to have in core and a marked improvement for interacting with this page.

a57c63d

thank you for your appreciation.