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 click modifiers, target prop #767

Merged
merged 13 commits into from Mar 19, 2020
Merged

Added click modifiers, target prop #767

merged 13 commits into from Mar 19, 2020

Conversation

@HammadTheOne
Copy link
Collaborator

HammadTheOne commented Mar 6, 2020

This PR addresses #748 and adds the ability to control+click a Link to open it in a new tab. Although html.A is a better component to use for external links, this adds that functionality for people who might use it within a multi-page Dash app.

This PR also adds in the target prop to add logic that one would expect from a traditional link.

Closes #748 .
Closes #770

@chriddyp

This comment has been minimized.

Copy link
Member

chriddyp commented Mar 6, 2020

Awesome! Could we do a little research on other single page app link components on GitHub and see if we are missing any other logic? We really want this link to look and feel like a link.

This article describes this a bit more: http://jgaskins.org/blog/2016/07/30/don-t-change-perceived-browser-functionality. It would be great to check out the source code of a popular component on GitHub that has gone through all of these different edge cases: 😸

From http://jgaskins.org/blog/2016/07/30/don-t-change-perceived-browser-functionality, they recommend this:

function handleClick(event) {
  var hasModifiers = (
    event.metaKey ||
    event.shiftKey ||
    event.altKey ||
    event.ctrlKey
  );

  // Only handle unmodified left click. Leave everything else alone.
  if(hasModifiers || event.button !== 1) return;

  event.preventDefault(); // Only prevent AFTER confirming you should handle this.
};
@HammadTheOne

This comment has been minimized.

Copy link
Collaborator Author

HammadTheOne commented Mar 6, 2020

That's a good point Chris. Added the other modifiers for now, I'll look for another similar component and see if we're missing anything else that might be nice to have.

@chriddyp

This comment has been minimized.

Copy link
Member

chriddyp commented Mar 6, 2020

Handling "enter" on focussed elements (you can "select" links by "tabbing" through the page) might be another one to consider. We might already handle this one effectively though.

@HammadTheOne

This comment has been minimized.

Copy link
Collaborator Author

HammadTheOne commented Mar 10, 2020

Added dcc.Link Title prop in #768 .

@Marc-Andre-Rivet Marc-Andre-Rivet added this to the Dash v1.10 milestone Mar 16, 2020
@HammadTheOne

This comment has been minimized.

Copy link
Collaborator Author

HammadTheOne commented Mar 18, 2020

Added in the external prop, of type bool. When set to True, the link will open in a new tab. While most Dash apps run in single-page, this combined with the other fixes to dcc.Location means a person can open up a dcc.Link in a multi-page app to a specific section of a chapter or page in a new tab. By default it is false, so it isn't a breaking change.

Closes #770 .

@@ -81,6 +87,7 @@ export default class Link extends Component {
href={href}
onClick={e => this.updateLocation(e)}
title={title}
target={external ? '_blank' : '_self'}

This comment has been minimized.

Copy link
@chriddyp

chriddyp Mar 18, 2020

Member

I think that I'd prefer that we just stick to the HTML convention here instead of defining a new API. That way, this component is more or less 1-1 with html.A. So, just let users pass through target directly rather than defining a new external property.

This comment has been minimized.

Copy link
@HammadTheOne

HammadTheOne Mar 18, 2020

Author Collaborator

I've seen a couple of other link components define external as a separate property, but I think you raise a good point, for both maintaining parity with html.A and offering a bit more freedom for the user to decide what to target.

Changed in 6514c83.

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Mar 18, 2020

Contributor

I think that I'd prefer that we just stick to the HTML convention here

I agree @chriddyp - cc @Marc-Andre-Rivet - we had a similar discussion a couple of days ago, also because there are other values of target that still matter.

@HammadTheOne HammadTheOne changed the title Added `ctrlKey` conditional Added click modifiers, target prop Mar 19, 2020
Copy link
Member

Marc-Andre-Rivet left a comment

💃

@HammadTheOne HammadTheOne merged commit b50ca8d into dev Mar 19, 2020
14 checks passed
14 checks passed
ci/circleci: build-dash-27 Your tests passed on CircleCI!
Details
ci/circleci: build-dash-36 Your tests passed on CircleCI!
Details
ci/circleci: build-dash-37 Your tests passed on CircleCI!
Details
ci/circleci: lint-unit-27 Your tests passed on CircleCI!
Details
ci/circleci: lint-unit-36 Your tests passed on CircleCI!
Details
ci/circleci: lint-unit-37 Your tests passed on CircleCI!
Details
ci/circleci: percy-finalize Your tests passed on CircleCI!
Details
ci/circleci: test-27 Your tests passed on CircleCI!
Details
ci/circleci: test-36 Your tests passed on CircleCI!
Details
ci/circleci: test-37 Your tests passed on CircleCI!
Details
ci/circleci: test-legacy-27 Your tests passed on CircleCI!
Details
ci/circleci: test-legacy-36 Your tests passed on CircleCI!
Details
ci/circleci: test-legacy-37 Your tests passed on CircleCI!
Details
percy/dash-core-components Visual review automatically approved, no visual changes found.
Details
@HammadTheOne HammadTheOne deleted the 748-control-link-fix branch Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.