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

fix hotkey trigger for clipboard sharing option #6277

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

jakopo87
Copy link
Contributor

@jakopo87 jakopo87 commented Apr 9, 2024

Clipboard sharing uses <button> instead of <a>, so the hotkey does not work because the selector does not find it.

Closes #6276

Changes proposed in this pull request:

  • Fix the selector used to enumerate sharing option;

How to test the feature manually:

  1. Enable clipboard sharing option;
  2. Use the corresponding numeric key of the clipboard sharing;
  3. Share dropdown should close and entry link copied to the clipboard;

Pull request checklist:

  • clear commit messages
  • code manually tested

Additional info:
The selector .dropdown-menu .item a, .dropdown-menu .item button can be used too, but it's more verbose than the one proposed.

Clipboard sharing uses  <button> instead of <a>, so the selector does not find it
Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

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

That'll be a recent regression. It was changed to <button> because it wasn't really a link. 👍

@Frenzie Frenzie added this to the 1.24.0 milestone Apr 9, 2024
@math-GH
Copy link
Contributor

math-GH commented Apr 9, 2024

Additional info: The selector .dropdown-menu .item a, .dropdown-menu .item button can be used too, but it's more verbose than the one proposed.

I prefer these selectors. It keeps the code cleaner&clearer. The data- attributes should be used in CSS as less a possible.

@Alkarex
Copy link
Member

Alkarex commented Apr 9, 2024

I prefer these selectors.

@math-GH Can you please made a code suggestion from the review page?

@math-GH
Copy link
Contributor

math-GH commented Apr 9, 2024

The solution made here with this PR does not work for Firefox. Also .dropdown-menu .item a, .dropdown-menu .item button does not work.

The Print sharing is broken too (it is also a button)

Sorry, my fault. But it is very important to know and understand: If the Default System Sharing is added to the list, it will not be displayed in all cases (f.e. invisible in Firefox on Win10, but visible in Firefox on Android). It will keeps its hotkey.

In my case, I pressed the 4 but it was the hidden Default System Sharing and not the desired Clipboard.
grafik

@@ -697,7 +697,7 @@ function auto_share(key) {
if (!share) {
return;
}
const shares = share.parentElement.querySelectorAll('.dropdown-menu .item a');
const shares = share.parentElement.querySelectorAll('.dropdown-menu .item [data-type]');
Copy link
Contributor

@math-GH math-GH Apr 9, 2024

Choose a reason for hiding this comment

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

Suggested change
const shares = share.parentElement.querySelectorAll('.dropdown-menu .item [data-type]');
const shares = share.parentElement.querySelectorAll('.dropdown-menu .item.share a, .dropdown-menu .item.share button');

Copy link
Contributor

@math-GH math-GH Apr 9, 2024

Choose a reason for hiding this comment

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

Another idea could be to add a share-action class on the <button> and <a> in \app\views\helpers\index\normal\entry_bottom.phtml. It would make the JavaScript line better:
const shares = share.parentElement.querySelectorAll('.dropdown-menu .item.share .share-action');

Copy link
Member

Choose a reason for hiding this comment

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

You could write it as .dropdown-menu .item.share :is(a, button) nowadays.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, but I do not like to travers on tags, I prefer to travers ids and classes

Copy link
Member

Choose a reason for hiding this comment

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

I think the original proposal with [data-type] does that better than adding a classname. [data-x] is clearly meant for JS and not for styling.

@Alkarex Alkarex merged commit 30f1474 into FreshRSS:edge Apr 11, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Numeric hotkey do not trigger clipboard sharing
4 participants