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

External link support #81

Merged
merged 4 commits into from
Mar 16, 2021
Merged

External link support #81

merged 4 commits into from
Mar 16, 2021

Conversation

merapi
Copy link
Contributor

@merapi merapi commented Mar 10, 2021

Numerous ways to do that, this one should do the job.

It was found in QA:
Finally, in the footer, there seems to be an error in clicking the "See the source code" link and there is no link to the Google Privacy Policy, T&S.

What T&S link should we use?

@merapi merapi self-assigned this Mar 10, 2021
@merapi merapi requested review from dero and derekherman March 10, 2021 22:46
Copy link
Collaborator

@dero dero left a comment

Choose a reason for hiding this comment

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

@merapi I don't think this is a good solution – it's an ad hoc rule that's easy to be forgotten and it leads to unexpected behavior and confusion. A standard looking link should always behave like a standard link.

Instead we should be explicit about what is a standard link and what is a "custom link handled by the Router". Something like <a href="..." data-use-router>...</a> would work, because it would make it obvious some kind of router is used to extend its functionality.

@merapi
Copy link
Contributor Author

merapi commented Mar 11, 2021

@merapi I don't think this is a good solution – it's an ad hoc rule that's easy to be forgotten and it leads to unexpected behavior and confusion. A standard looking link should always behave like a standard link.

Instead we should be explicit about what is a standard link and what is a "custom link handled by the Router". Something like <a href="..." data-use-router>...</a> would work, because it would make it obvious some kind of router is used to extend its functionality.

2 nits:
A standard looking link should always behave like a standard link. - that's not the case for most of the FE routers that I saw (react/vue router etc.).
it's an ad hoc rule that's easy to be forgotten and it leads to unexpected behavior and confusion - you can say the same about data-use-router and because it's a SPA, it's more likely that you will encounter that confusion.

In case of SPA that condition should be flipped IMHO.
So that would mean adding an "external" attribute to those 2 links. How about that?

@merapi merapi requested a review from dero March 11, 2021 10:29
@dero
Copy link
Collaborator

dero commented Mar 11, 2021

that's not the case for most of the FE routers that I saw (react/vue router etc.).

@merapi Correct me if I'm wrong, but don't both routers use a custom element to define a router link in templates? We don't compile our templates, so I would be inclined to think our situation is different.

@merapi
Copy link
Contributor Author

merapi commented Mar 11, 2021

that's not the case for most of the FE routers that I saw (react/vue router etc.).

@merapi Correct me if I'm wrong, but don't both routers use a custom element to define a router link in templates? We don't compile our templates, so I would be inclined to think our situation is different.

That just depends on the context, and we didn't had the same.

@merapi
Copy link
Contributor Author

merapi commented Mar 11, 2021

Now with data-no-router solution.

Copy link
Collaborator

@dero dero left a comment

Choose a reason for hiding this comment

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

I maintain that custom routing should be opt-in when standard hyperlinks are used for navigation – not opt-out.

The <a href="URL"> element has a clear default action – to navigate to a given URL. The opt out approach is going to lead to situations where this experience is broken even for valid URLs simply by omitting the opt out attribute.

We may also not always be in control of the markup that is rendered – one can imagine video descriptions that are allowed to contain hyperlinks, it's a rather normal use case. In that situation we would need to add the opt out attribute on the fly. And if we did that, we wouldn't be able to serve an internal link this way. The system feels inherently unstable.

I've also re-checked the major front-end frameworks and their router implementation to verify that I'm not missing the mark here by requesting something unreasonable or illogical:

  • React and Vue routers use a custom element.
  • Angular router uses <a> with a custom attribute to define the route.
  • Svelte SPA router uses <a> with a custom use:link attribute to invoke the router or alternatively a # preceding the route.

public/index.html Outdated Show resolved Hide resolved
@derekherman
Copy link
Collaborator

I maintain that custom routing should be opt-in when standard hyperlinks are used for navigation – not opt-out.

I agree 100% and this should be fixed.

@merapi merapi requested review from derekherman and dero March 15, 2021 13:14
@merapi
Copy link
Contributor Author

merapi commented Mar 15, 2021

Updated with data-use-router approach, please re-check.

dero
dero previously approved these changes Mar 16, 2021
Copy link
Collaborator

@dero dero left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@dero dero merged commit 7c50b94 into develop Mar 16, 2021
@dero dero deleted the add/external-links branch March 16, 2021 06:46
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