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

[popup] Add option to open extension pages in new tab using keyboard shortcuts #158

Merged

Conversation

toly11
Copy link
Contributor

@toly11 toly11 commented Sep 17, 2023

Description
In the current implementation, navigating to the Export and Import pages through keyboard shortcuts (Ctrl+Option+I on Mac or Ctrl+Alt+I on Windows, followed by E or I) only allows users to open these pages in the current tab.
Opening these pages in the current tab disrupts the user flow
In order to open the page in a new tab, users have to resort to using a combination of keyboard commands and mouse clicks which is not ideal. (control or command + mouse click).

Enhancement
This pull request introduces an enhancement to the existing keyboard shortcut functionalities, allowing users to open the extension pages in a new tab seamlessly. By holding the Command key (on Mac) or Control key (on Windows) in conjunction with the existing shortcut keys, users can now open the extension pages in a new tab, thereby preserving their position on the current page and facilitating a smoother navigation experience.

Testing
This feature has been tested across different environments to ensure compatibility and smooth integration with existing functionalities. I encourage further testing to validate its efficiency and usability enhancements.

Request for Review
I kindly request a review of this pull request and am open to making any necessary adjustments based on your feedback. Thank you for considering this valuable enhancement to the user experience.

@tprouvot
Copy link
Owner

Hi @toly11 ,
Thanks for this PR !

At first I was thinking that is functionality il already covered by this one.
In fact not really because a user can usually prefer to open link in the current tab and for specific needs in a new tab, so thanks for addressing a new use case !

This being said, it means that the shortcuts did not use the parameter I added in v1.17 to open all links into a new tab.

Could you update your PR to:

  • Add your feature to CHANGES.md following existing naming convention
  • Create a function to use both options (localVariable and the ctrl or meta key)
getLinkTarget(e) {
    if (localStorage.getItem("openLinksInNewTab") == "true" || (e.ctrlKey || e.metaKey)){
      return "_blank";
    } else {
      return "_top";
    }
  }

This new functionality will be hidden and can be missed if someone does not read the release note or checks the PR.
Since few releases, I'm trying to improve documentation on this extension, it could be great to add a new section to the 'How to' page and describe what can be done with links and new/current tab.
What do you think ?

If you're interested, you can update the file in this PR.

Respect user preference for link targets
@toly11
Copy link
Contributor Author

toly11 commented Sep 18, 2023

Hey @tprouvot,
Thank you for reviewing this PR.

I have update the CHANGES.md file and moved the logic to a separate function. I chose to write it as a separate function instead of as a class method since I can see this code being used by other components too in the future.

I think it could be great if there will be a general documentation area for all the extension's features which can include a "how to" section, If that makes sense

@tprouvot tprouvot changed the title Add option to open extension pages in new tab using keyboard shortcuts [popup] Add option to open extension pages in new tab using keyboard shortcuts Sep 27, 2023
addon/popup.js Outdated
@@ -79,29 +79,32 @@ class App extends React.PureComponent {
}
if (e.key == "e") {
e.preventDefault();
this.refs.dataExportBtn.target = (e.ctrlKey || e.metaKey) ? "_blank" : "_top";
Copy link
Owner

Choose a reason for hiding this comment

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

Use the created function to get the target

Suggested change
this.refs.dataExportBtn.target = (e.ctrlKey || e.metaKey) ? "_blank" : "_top";
this.refs.dataExportBtn.target = this.getLinkTarget(e);

addon/popup.js Show resolved Hide resolved
Remove extra breakline
@tprouvot tprouvot merged commit b652608 into tprouvot:releaseCandidate Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants