Skip to content
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

added copy url button #8806

Merged
merged 3 commits into from
Feb 29, 2024
Merged

Conversation

Ayush1404
Copy link
Contributor

Closes #8802

Adds way to copy the URL of the page you're on to share it.

Technical

Testing

Go to any books page and click on share.

Screenshot

Screenshot from 2024-02-08 20-28-16

Stakeholders

@mekarpeles

@mekarpeles mekarpeles self-assigned this Feb 12, 2024
@mekarpeles
Copy link
Member

This seems to work though it's unclear to the patron whether the copying has succeeded. We either need some message or to show the text in a popup, like the "embed" option.

Copy link
Member

@mekarpeles mekarpeles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some confirmation required (e.g. a toast message or popup) that the url copying succeeded

@mekarpeles
Copy link
Member

@jimchamp is this a good place to use toast? Do we have an example of doing this via js?

@mekarpeles mekarpeles assigned jimchamp and unassigned mekarpeles Feb 26, 2024
@jimchamp
Copy link
Collaborator

@jimchamp is this a good place to use toast? Do we have an example of doing this via js?

Yes, and the modal should close when the URL copy icon is clicked. Will provide guidance shortly...

Copy link
Collaborator

@jimchamp jimchamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @Ayush1404! Happy to merge this once the suggested changes have been made.

You may already be aware of this, but when any .less or .js file is modified, the CSS and JS assets must be recompiled. There is more information about this here.

Let me know if you have any questions about anything.

<a class="copy-url-btn" title="$_('Copy url to clipboard')" data-ol-link-track="Share|CopyURL"
onclick="navigator.clipboard.writeText('openlibrary.org'+'$page_url');">
<img
alt="CopyURL icon"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
alt="CopyURL icon"
alt="$_('Copy URL icon')"

Add space between Copy and URL. Use localized strings (more information about this here).

Comment on lines 54 to 55
<a class="copy-url-btn" title="$_('Copy url to clipboard')" data-ol-link-track="Share|CopyURL"
onclick="navigator.clipboard.writeText('openlibrary.org'+'$page_url');">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<a class="copy-url-btn" title="$_('Copy url to clipboard')" data-ol-link-track="Share|CopyURL"
onclick="navigator.clipboard.writeText('openlibrary.org'+'$page_url');">
<a class="copy-url-btn" title="$_('Copy url to clipboard')" data-ol-link-track="Share|CopyURL">

Move inline JS to Webpack. To do so, you can update this function to do the following:

  1. Get a reference to the "Copy URL" icon. This query should work #social-modal-content .copy-url-btn.
  2. Add a click listener to the icon.

The click listener should:

  1. Copy the URL to the clipboard.
  2. Close the modal. (Running $.colorbox.close() will achieve this)
  3. Display a toast message that tells the patron that the message has been copied to their clipboard. You can use this function to display the toast message (only pass message --- $parent is not needed).

@jimchamp jimchamp added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Feb 26, 2024
@Ayush1404
Copy link
Contributor Author

Hey @jimchamp, I have done all the corrections suggested by you. While doing so I realized that embed-work-btn button also has some js in Share_Modal.html Here . Should I move that code also to Webpack?

$('#social-modal-content .copy-url-btn').on('click', function(event){
event.preventDefault();
navigator.clipboard.writeText(window.location.href);
showToast('URL copied successfully')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jimchamp question, is there a way we handle i18n in JS like this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I'll include that in the follow-up issue (mentioned here).

@jimchamp
Copy link
Collaborator

Hey @jimchamp, I have done all the corrections suggested by you. While doing so I realized that embed-work-btn button also has some js in Share_Modal.html Here . Should I move that code also to Webpack?

Thanks for pointing that out. Let's consider this out of scope for this PR. As a follow-up, I'll open an issue for this.

@mekarpeles mekarpeles added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Feb 29, 2024
Copy link
Collaborator

@jimchamp jimchamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @Ayush1404!

I'm going to commit my small suggested changes, then merge this branch.

openlibrary/plugins/openlibrary/js/modals/index.js Outdated Show resolved Hide resolved
Comment on lines +13 to +15
/**
* Adds click listeners to buttons in all notes modals on a page.
*/
Copy link
Collaborator

@jimchamp jimchamp Feb 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* Adds click listeners to buttons in all notes modals on a page.
*/
/**
* Adds click listener which copies the current page's URL to the clipboard.
*/

openlibrary/plugins/openlibrary/js/modals/index.js Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 16.02%. Comparing base (f0796d7) to head (d965477).
Report is 121 commits behind head on master.

Files Patch % Lines
openlibrary/plugins/openlibrary/js/modals/index.js 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8806      +/-   ##
==========================================
- Coverage   16.62%   16.02%   -0.61%     
==========================================
  Files          88       89       +1     
  Lines        4698     4693       -5     
  Branches      838      818      -20     
==========================================
- Hits          781      752      -29     
- Misses       3399     3434      +35     
+ Partials      518      507      -11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jimchamp jimchamp merged commit 9915746 into internetarchive:master Feb 29, 2024
4 checks passed
@jimchamp
Copy link
Collaborator

I ended up moving the inline embed html code to Webpack in #8853.

@Ayush1404 Ayush1404 deleted the add-copy-url-button branch March 1, 2024 00:36
@jimchamp jimchamp removed the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "Copy URL" Option to Share Modal
4 participants