Opened 6 years ago
Closed 4 years ago
#44588 closed enhancement (fixed)
Denote the Copy action is complete by updating the Copy button to state 'Copied'
Reported by: | garrett-eclipse | Owned by: | garrett-eclipse |
---|---|---|---|
Milestone: | 5.5 | Priority: | normal |
Severity: | normal | Version: | 4.9.6 |
Component: | Privacy | Keywords: | has-patch has-screenshots commit |
Focuses: | administration | Cc: |
Description
Hello,
With the Suggested Privacy content introduced in 4.9.6 the sections provide a 'Copy' button;
https://i.imgur.com/WP8JmEw.png
It would be nice if once the action is complete that the button would update to state 'Copied' briefly before switching back to 'Copy' for more copying. By switching to 'Copied' temporarily the users gets direct feedback that the copy action was completed so they don't keep clicking the button unnecessarily.
Chers
Attachments (16)
Change History (49)
This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.
6 years ago
#2
@
6 years ago
- Focuses administration added; privacy removed
- Keywords ui-feedback added
- Milestone changed from Awaiting Review to Future Release
Good find, @garrett-eclipse. This should definitely get improved. I think adding user feedback to indicate the copy happened successfully is a good idea, but I also don't know that there is enough information here. What should the user do after copying? What does "Copy" actually do? Maybe the button could be "Copy to Clipboard".
I am marking this as ui-feedback
to explore this a bit more.
#3
@
6 years ago
It's also worth noting that there is one copy button for the entire suggested text provided with core. Each plugin that suggests text will receive their own copy button. Maybe "Copy Section" after each section, or a way to bulk select the sections to copy would be useful.
#5
@
6 years ago
The "Email Data" button, on the privacy export data page, comes to mind, as it gives an immediate feedback when it's clicked on.
But it can only be clicked once, for each page load.
I think we should be able to click many times on the Copy button, without reloading the page.
I wonder about the following flow, with a javascript timer:
- Copy button is clicked.
- Button text changes immediately to "Copied".
- After
n
milliseconds, the button text is back to "Copy".
Another one is to have a label near the button showing the state, but it should be cleared when the user copies other sections or makes other manual copies of text on the page. For a very long text (many plugin sections), this might be slower than the first option.
Third option could be to not show any feedback at all (status quo).
ps:
Here's an interesting discussion I found on the topic:
#6
@
6 years ago
- Keywords ux-feedback added
Thanks @birgire, I do like the idea of providing feedback directly in the button there so maybe the timer is the best idea.
There was an idea in your link I thought interesting of checking the clipboard to see if the contents is in there and while it's there keep the button as 'Copied' until the clipboard changes.
#7
@
5 years ago
- Keywords needs-patch added; ui-feedback ux-feedback removed
- Milestone changed from Future Release to 5.3
- Owner set to garrett-eclipse
- Status changed from new to assigned
This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.
5 years ago
This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.
5 years ago
This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.
5 years ago
#12
@
5 years ago
- Milestone changed from 5.3 to 5.4
@garrett-eclipse This ticket still needs a patch. With version 5.3 Beta 1 landing tomorrow, this enhancement is being moved for consideration in 5.4. If this can be patched for 5.3, feel free to update the ticket.
#13
follow-up:
↓ 28
@
5 years ago
Rather than open a new ticket wanted to flag the copy to TextEdit & Microsoft Word should be tested once clipboard.js is adopted here to ensure the unnecessary formatting is cleaned up.
[Edit] In the attached screens below the main unnecessary formatting that comes over is the background and empty space.
This ticket was mentioned in Slack in #core-privacy by nickylimjj. View the logs.
5 years ago
This ticket was mentioned in Slack in #core-php by nickylimjj. View the logs.
5 years ago
#16
@
5 years ago
- Keywords has-patch has-screenshots added; needs-patch removed
A patch and 2 pictures to demonstrate the patch. Perhaps to review
- style design for the 'Copied!' feedback. Where should it be?
- Having a challenge to get the formatting right for the copied data since I accessed the data through internal variables (that is in html) because it lets me
- parse out the
description
- not leave a highlighted side-effect on the text on button click
- parse out the
#17
@
5 years ago
- Keywords needs-testing dev-feedback 2nd-opinion added
Thanks for the patch @nickylimjj this is a great start here.
Reviewing I made a few minor tweaks in 44588.3.diff, they are;
- Cleanup JS variable creation.
- Updated JS a11y.speak text to use 'Privacy Policy Guide section' instead of 'Privacy Content' to be more specific.
- Reverted the css class for
.privacy-text-copy-button
back to.privacy-text-copy
in case plugins have styled or targetted it. This is to avoid back-compat issues. - Cleanup PHP for coding standards on indentation.
- In the PHP removed unneeded variable instantiations when they are stings only being used a single time.
I'm adding dev-feedback
/ 2nd-opinion
to get thoughts on the copy formatting change as you noted it now copies HTML instead of formatted text. From my testing this works ok in the block editor as it gets converted to blocks, but in the classic editor and classic block it's pasted as HTML so you'd need to use the Text tab to paste. As well pasting to rich text editors like Word, Pages, etc no longer holds the formatting. Personally I'm on the fence as I wasn't a huge fan of the original formatted copy but now it's less transferrable.
*Possibly we can place the headings and paragraphs on new lines so it's less a jumble and easier to read.
#18
@
5 years ago
P.S. I forgot to address your placement question, I agree having the Copied text close to the button makes the most sense and avoids it being overlooked on large screens. My patch is an enhancement to that version.
#19
@
5 years ago
Hey @nickylimjj & @garrett-eclipse, thanks for the patches!
I've checked both 44588.2.diff and 44588.3.diff and they look promising but there are a couple of issues that need to be addressed to get this finalized.
The existing code is targetting the nearest "wp-suggested-text" and copies its inner code. Since the patches introduce the clipboard.js the copied information has to be added to the button.
The code on the patches currently is as:
$content .= sprintf( '<button type="button" class="privacy-text-copy button" data-clipboard-text="%1$s">', esc_attr( self::get_default_content( false, false ) ) );
This means that all clipboard buttons are forced to copy the default text due to self::get_default_content( false, false )
. This should be changed to $section['policy_text']
instead so each button has it's own section text available to get copied.
Also, note that since "everything" will actually be copied now the clipboard text ends up having the parent <div class="wp-suggested-text">
wrapper which is not needed.
As far as I can see from various plugins that I've installed to test this we most likely need to strip the wrapper mentioned as well as all the class="privacy-policy-tutorial"
as they are not needed for copying and they might interfere with any front-end styling. This should leave a clean text with just headings and paragraphs for copying.
I hope this makes sense & tell me if you need any help with bumping a .4 patch with these changes so I can take a closer look!
#20
@
5 years ago
Following up after a brief DM over Slack and looking further into this.
I played around with copy/pasting on various mediums (offline/online document editors) & Gutenberg.
It seems that there are some issues when using the data-clipboard-text
due to various tabs and all other special chars used in each section here.
I've tried converting the text striping out anything that's not needed like "\r\n" and tabs etc but then the copy/paste styling seemed to not be respected by all mediums. I couldn't pinpoint why but there wasn't any need to go further with this approach since it kinda broke from scratch. Note that the issue was for the policies that plugins might add, the default policy worked just fine :).
Also, the text
data-clipboard parameter practically adds all of the text that we wanted to copy again in the HTML source and that didn't seem right to me since there's no difference in the HTML as we actually want to keep the formatting here. Essentially on this implementation, the data-clipboard-text
was printing the same content a second time just for the usage of the copy method creating a bigger HTML source without any actual need.
In 44588.4.diff I've converted the patch to use data-clipboard-target
instead of the data-clipboard-text
.
Each section now has its own unique ID that is generated by the source name like id="policy-text-PLUGINNAME"
. This makes it easier to add targets to clipboard.js so each copy button can target the specific section it belongs to and copy it.
This way there's no need to output double HTML inside data placeholders and the styling/formatting seems to be working fine on various mediums / Gutenberg / Google Docs as expected.
#21
@
5 years ago
After checking #44621 I realized (as I had forgotten about it, sorry!) that the current Copy action only grabs the headings + suggested texts from each section.
On the other hand, the patches introduced here ( including mine :) ) copy "everything" including notes from authors and explanatory texts that are not needed from the incoming texts from plugins.
I tried playing around a bit with clipboard.js trying to make it copy only the suggested texts as needed but to no avail.
If anyone else likes to give this a try feel free to do so, but if we can't make it work with clipboard.js I'd suggest leaving the functionality as is at the moment to only copy the "suggested text" as needed and simply add the "Copied!" notice.
@garrett-eclipse would you be ok with that instead?
#22
@
5 years ago
Hey @xkon!
The initial patches (up to .3) that uses data-clipboard-text
manage to exclude the description
(refer to clipboard-data.png) because the first parameter in self::get_default_content( false, false )
can control that. I considered data-clipboard-target
but I had the same problem you encountered in your .4 patch + it left a side effect on my browser (the text was highlighted, as if you dragged your cursor across).
Perhaps we would want to redefine the$section['policy_text']
data structure for more effective targeting? Otherwise, another strategy could be to create a function that parses $section['policy_text']
since it's already expressive as HTML.
#23
@
5 years ago
Hey @nickylimjj, thanks for the follow-up!
Unfortunately, the text that is added at the moment is a little bit "vague" in regards of the HTML as we didn't set any standard classes to somehow distinguish what is explanatory vs suggestion. This has led to plugins adding just paragraphs and heading without any major distinction though which is kind of making it "non-expressive".
The difference that was visible currently with the current italic Suggested text was that it was just wrapped in an extra <p> without any class. This is also under consideration at the moment on changing in #44621 that could potentially add a wrapper around the suggested text only for better visual indication so that would definitely make targetting way easier for the copying purposes.
What I propose for now to avoid losing the scope here also is to simply add the functionality needed for this ticket only which is the indication of "Copied!".
Moving to clipboard.js and changing the actual copy mechanism should be tracked on a different ticket than this, at least that's what I propose to not end up having tickets again waiting on each other that are changing functionality without it being obvious.
This ticket was mentioned in Slack in #core-privacy by xkon. View the logs.
5 years ago
#26
@
5 years ago
Thanks @nickylimjj for .5 patch :) !
44588.6.diff goes 1 step further following 44588.5.diff and also adds the i18n as well as a11y.speak functionality.
@garrett-eclipse I think we're OK with this feel free to take another look if anything else might be needed else I'm +1 for commit
tag.
@
5 years ago
Refresh for a few minor items and made the Copied text dismissable after 3 seconds so additional clicks will receive the indication their text was copied.
@
5 years ago
GIF to illustrate the vanishing Copied text. This is so additional clicks get the same feedback that something occurred.
#27
@
5 years ago
- Keywords dev-feedback 2nd-opinion removed
- Status changed from assigned to accepted
Thanks @nickylimjj & @xkon for the work here. Sorry my suggestion to adopt ClipboardJS didn't pan out. I appreciate you both delving into it fully before abandoning that approach. I agree after testing those patches the clipboard contents was never really up to par with what we had originally so am all onboard with the current direction.
In 44588.7.diff I've made a minor refresh to re-apply some changes from my comment:17 mainly;
- Cleanup JS variable creation.
- Cleanup PHP for coding standards on indentation.
- In the PHP removed unneeded variable instantiations when they are strings only being used a single time.
I also updated the copy action to introduce a setTimeout
in order to dismiss the Copied!
text after 3 seconds. This is so when coming back to a used button and clicking copy again we get the feedback. When we didn't dismiss the text any additional clicks don't receive that visual feedback and so aren't sure if the action completed. Demo;
This feels good to go to me but I don't want to mark my own patch for commit
, could either of you (@nickylimjj or @xkon) give my latest patch a once over and test and we'll put our stamp of approval on this. My tests, CS checks and PHPUnit tests all ran green.
Sidenote: While testing with some plugins such as WooCommerce I noticed they were still using the old .wp-policy-help
class which resulted in their help/tutorial text being included in the copy content. To address this I opened #49282.
#28
in reply to:
↑ 13
@
5 years ago
Replying to garrett-eclipse:
Rather than open a new ticket wanted to flag the copy to TextEdit & Microsoft Word should be tested once clipboard.js is adopted here to ensure the unnecessary formatting is cleaned up.
[Edit] In the attached screens below the main unnecessary formatting that comes over is the background and empty space.
Revisiting this comment I didn't want to overlook this so in order to keep this ticket moving forward have created #49283 to address the empty lines where the tutorial text was hidden during the copy action.
#29
@
5 years ago
- Keywords commit added; needs-testing removed
I've retested the final patch here and works as expected on my end as well, marking this for commit. Thanks for the refresh!
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
5 years ago
#31
@
5 years ago
- Keywords needs-refresh added; commit removed
- Milestone changed from 5.4 to Future Release
This needs a refresh as it seems after other various recent commits.
I'm marking this for a refresh & future release since we're so close on 5.4 Beta at the moment.
#32
@
5 years ago
- Keywords commit added; needs-refresh removed
- Milestone changed from Future Release to 5.5
I've refreshed the patch in 44588.8.diff, let's get this in for 5.5.
Copy Button