search

scullyio / scully

The Static Site Generator for Angular apps
https://scully.io/
MIT License
2.55k stars 257 forks source link

Links in scully blogs do not respect Ctrl Click #1662

Open hisham opened 1 year ago

hisham commented 1 year ago

🐞 Bug report

Description

It seems for blog posts, scully takes over onclick behaviour in links, and if scully is aware of the route, it overrides the link navigation via this.router.navigate.

However, this means that ctrl-click to open link in new tab is not respected. target="_blank" is not respected either. Also I notice sometimes when user presses back, their location in the article does not remain the same either.

The relevant code is here: https://github.com/scullyio/scully/blob/179650fc26b8951e0eaedf9edc0a0f6b9a49fc8b/libs/ng-lib/src/lib/scully-content/scully-content.component.ts#L194

🔬 Minimal Reproduction

Create angular scully app with blog post that links to a route in the app that scully is aware of.

💻Your Environment

Angular Version: 15

Scully Version: 2.1.41

hisham commented 1 year ago

Is this upgradeToRouteLink feature needed anymore btw? My links seem to work fine without it, and does not reload the page if I link within the angular app.

hisham commented 1 year ago

related to https://github.com/scullyio/scully/issues/1615 as well. I think there should be an Input to disable the scully routing upgrade for all links...

SanderElias commented 1 year ago

@hisham, without the upgrade, your links don't stop working. However, they will do a full-page refresh navigation. I'm unsure that if you use a [route-link]='somelink' one can do the right click either?

hisham commented 1 year ago

Hi @SanderElias - thanks for the response. Ok, yes I do see the page refresh navigation when I remove Scully's onclick handler. While not having page refresh is ideal, to be honest, it's not so bad.

To answer your second question, yes angular's routerLink directive does respect ctrl-click. Looks like they supported this since 2016 - see https://github.com/KiaraGrouwstra/angular/commit/1ac9dda93d01e5e61a3af32a9c596e6f26d50b00 for the fix they implemented to support this. They catch the mouse event and if meta or ctrl key was pressed at same time, they let the browser do its thing and do not handle anything. So I guess Scully can do that as well maybe. Notice as well they look at the target attribute as well.

However, another issue I found is when I click on one of the links in the blog post and then go back, the scroll position in the original blog post page is not preserved. I haven't spent the time investigating why but I suspect it's due to how the scully-content component dynamically inserts blog content.

hisham commented 1 year ago

yeah I'm pretty sure scroll positioning is caused by scully's dynamic blog post content insertion. It seems to be done in a non-angular way, so maybe that's why angular is unable to preserve scroll history for blog posts. Scully might need to remember scroll positions itself like what is proposed here: https://dev.to/johncarroll/angular-how-to-refresh-scroll-position-on-back-5e1b