-
Notifications
You must be signed in to change notification settings - Fork 71
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
webui: Improve UI of links for viewing search results in context. #515
Conversation
@Henry8192 Please help triage this review. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question.
{" "} | ||
<a | ||
className={"search-results-file-link"} | ||
rel={"noopener noreferrer"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning behind setting these? Fwiw, _blank
seems to imply noopener
now:
Note: Setting target="_blank" on elements implicitly provides the same rel behavior as setting rel="noopener" which does not set window.opener.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah.. I didn't read the docs when I wrote this.
Apparently the behaviour change is fairly recent (with Chrome adapting it only since 2021) so we might better keep the noopener
:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#browser_compatibility:~:text=target%3D%22_blank%22%20implies%20rel%3D%22noopener%22%20behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the PR title, how about:
webui: Improve UI of links for viewing search results in context.
Description
Validation performed
Sample screenshot: