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

Open external links on new tab #269

Merged
merged 7 commits into from
Jul 6, 2023
Merged

Open external links on new tab #269

merged 7 commits into from
Jul 6, 2023

Conversation

Aprillion
Copy link
Collaborator

Code cleanup & fixing #222

image

Testable on https://stampy-ui.aprillion.workers.dev/

@Aprillion
Copy link
Collaborator Author

Aprillion commented Jul 5, 2023

aaah, we don't use post css and Chrome only supports -webkit-mask, need to update 😓

Copy link
Collaborator

@mruwnik mruwnik left a comment

Choose a reason for hiding this comment

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

Approved, because the code seems correct, and clicking on the links works.
At the same time, I'd like to note that I agree with your initial doubts, and so am disappointed at how you didn't wait for someone else to implement it, hence failed condemning it to languish in the backlog :P

@ccstan99
Copy link
Collaborator

ccstan99 commented Jul 5, 2023

Sorry to be the pain but my fault for reviving the languishing zombie. Unfortunately, it's the right thing to do from a user's expectation point of view and for metrics, although it'll open a different set of issues for editors to rethink how they structure content and links.

@Aprillion
Copy link
Collaborator Author

Aprillion commented Jul 6, 2023

This is part of our code that wouldn't make a good first issue to welcome new developers, and it was deemed important by many stakeholders in our team, so I decided to implement it so the issue stops haunting us 🤷‍♂️

@mruwnik feel free to add a condition like if (!localStorage.get('disableExternalLinksOpenOnNewTab')) if you think it might be important to have an option to disable the behaviour (and we could ask to include it as a user-visible setting in the UI redesign later this year)

@Aprillion Aprillion merged commit 70b1535 into master Jul 6, 2023
@Aprillion Aprillion deleted the links-new-tab branch July 6, 2023 06:01
@ccstan99
Copy link
Collaborator

ccstan99 commented Jul 6, 2023

@Aprillion @mruwnik Just so I better understand your concerns... Is the problem nasty code maintenance? Or users overwhelmed with too many new tabs? If it's the latter, we could ask the user which they prefer ("Open in new tab or replace current one?" with an option "Remember this setting." Or does require cookies that we need to avoid?

@mruwnik
Copy link
Collaborator

mruwnik commented Jul 6, 2023

no, it's more about changing standards. Or leaving things as default as possible. It's part of the general process of websites taking on more and more of the basic functionality of browsers, which can be helpful, but often just makes things more counter intuitive. Though this may be a matter of "get off my lawn!!"? :D

It takes away freedom in the name of temporary comfort. And breaks one of the potential user flows.

I don't think having millions of options like this would help much. That's going back to badly reimplementing the browser in JS.

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