Skip to content

Implement in-app transitions for native anchor elements so that LinkTo isn't needed#19271

Closed
NullVoxPopuli wants to merge 1 commit intoemberjs:mainfrom
NullVoxPopuli:rfc-391-anchor-global-dispatcher
Closed

Implement in-app transitions for native anchor elements so that LinkTo isn't needed#19271
NullVoxPopuli wants to merge 1 commit intoemberjs:mainfrom
NullVoxPopuli:rfc-391-anchor-global-dispatcher

Conversation

@NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Nov 14, 2020

Adds support for using solely <a> tags for in-app navigation.

Ref: emberjs/rfcs#391

Tested using: NullVoxPopuli/ember-tutorial-link-component#1

@NullVoxPopuli NullVoxPopuli force-pushed the rfc-391-anchor-global-dispatcher branch from 9323083 to a43baec Compare November 14, 2020 17:32
@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review November 15, 2020 16:50
@NullVoxPopuli NullVoxPopuli force-pushed the rfc-391-anchor-global-dispatcher branch from a43baec to a312de6 Compare November 15, 2020 16:50
@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Nov 15, 2020

Questions:

@NullVoxPopuli NullVoxPopuli force-pushed the rfc-391-anchor-global-dispatcher branch from a312de6 to ffabd74 Compare November 15, 2020 22:03
per RFC emberjs#391, using the global event_dispatcher
@NullVoxPopuli NullVoxPopuli force-pushed the rfc-391-anchor-global-dispatcher branch from 313c403 to da374aa Compare November 15, 2020 22:39
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

@bobisjan - Mind taking a look? We were mostly fleshing this out over in adopted-ember-addons/ember-router-helpers#267 (essentially as a polyfill).

@rwjblue
Copy link
Member

rwjblue commented Nov 16, 2020

I think the same issue that was mentioned in the last comment over there is an issue here too. https://emberjs.github.io/rfcs/0391-router-helpers.html#route-globs-and-route-blacklisting

<a class="deep-nest" href="/blog/1/some-param">Deeply Nested</a>
<a class="QPs" href="/blog/1/some-param?applicationQP=1&q=2&foo=3">with QPs</a>
<a class="QPHash" href="/blog/1/some-param?applicationQP=1&q=2&foo=3#target">with QPs and hash</a>
<a class="hash" href="/blog/1/some-param#target">with hash</a>
Copy link
Contributor

@lifeart lifeart Nov 16, 2020

Choose a reason for hiding this comment

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

should we add bad cases?

<a href="javascript:void(0)" id="loginlink">login</a>
<a href="#id-on-page">to some block</a>
<a href="#" onclick="return false;">hello</a>
<a href="backup_page_displaying_image.aspx" onclick="return coolImageDisplayFunction();">hello</a>
<a href="https://github.com/robots.txt">some file</a>
<a href="blob:..." download="name">some programmatic download button</a>

in addition, if we have base (rootUrl) property for app, should we skip links upper to it? https://blog.emberjs.com/2016/04/28/baseurl

@GCheung55
Copy link
Contributor

I think this is a good addition and could replace ember-href-to addon. However, this change does not allow the exclusion of links which has been very useful through ember-href-to where I am working on a project where a page is being migrated, allowing development with the same URL as the old page is still being supported.

E.g.
Production has /foo/bar served by a server.
Development app is replacing /foo/bar, which should be handled by the app.
On production, links to /foo/bar from within the Ember app is not handled as an in-app transition, allowing the old page to still be navigatable until the page is enabled/migrated.

@NullVoxPopuli
Copy link
Contributor Author

@GCheung55 if the link isn't present in your app, it'll fallback to normal anchor behavior.

Is there something else I'm missing?

@mehulkar
Copy link
Contributor

I thought I saw somewhere that the goal was to get rid of EventDispatcher?

@GCheung55
Copy link
Contributor

@GCheung55 if the link isn't present in your app, it'll fallback to normal anchor behavior.

Is there something else I'm missing?

What I tried to describe is that the link DOES exist in the app but still needs to maintain normal anchor behavior as a path to a seamless migration from a non-Ember page to an Ember page.

I'll try to describe in terms of BDD:

GIVEN that a path /foo exists as a non-Ember page and an Ember page
AND a web server controls routing traffic to non-Ember pages and Ember pages
WHEN navigating to /foo, in a non-Ember page or Ember page
THEN a full page load occurs and loads the non-Ember page.

GIVEN that the web server is updated to direct /foo traffic to Ember pages
WHEN navigating to /foo, in a non-Ember page or Ember page
THEN a full page load occurs and loads the Ember page.

^ This is existing behavior that I can control with ember-href-to addon. Once the web server has been updated, the links to /foo in the Ember app are updated to be in-app navigation. The PR doesn't support this scenario described above.

@kategengler
Copy link
Member

Is this still relevant?

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Dec 12, 2023

it's desperately needed for DX and "using the platform"

Incorrect link, because I thought this was something else

it's a feature that's desperately needed, else hacks get implemented:
https://github.com/CrowdStrike/ember-url-hash-polyfill/blob/main/addon/index.ts

But maybe a new Polaris router would do this for us somehow?

@kategengler
Copy link
Member

You have an open issue to amend an RFC, so I wasn't sure if this particular implementation was still relevant.

@NullVoxPopuli
Copy link
Contributor Author

so I wasn't sure if this particular implementation was still relevant.

I am not sure, but also, I implemented this in userland here:

https://ember-primitives.pages.dev/4-routing/proper-links

Hopefully the new router can have this built in.

@NullVoxPopuli NullVoxPopuli deleted the rfc-391-anchor-global-dispatcher branch December 12, 2023 20:12
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.

6 participants