-
Notifications
You must be signed in to change notification settings - Fork 1
HEEDLS-465 - Made help and github links open in new tabs #476
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
Conversation
DigitalLearningSolutions.Web/ViewModels/Common/ViewComponents/SideMenuLinkWithHrefViewModel.cs
Outdated
Show resolved
Hide resolved
| } else { | ||
| <li class="side-nav__item"> | ||
| <a class="side-nav__link" href="@Model.Href" asp-all-route-data="@Model.AspAllRouteData">@Model.LinkText</a> | ||
| <a class="side-nav__link" href="@Model.Href" asp-all-route-data="@Model.AspAllRouteData" target="@(Model.OpenInNewTab ? "_blank" : null)"> |
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 - we should probably add "noopener" to the true external links (GitHub):
https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types/noopener
I wonder if it might be worth not reusing this component for these links in that case, so we can keep the component simple by not adding two extra properties just for two extra use cases.. Thoughts? 🤔
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.
Makes sense. Do we think there won't be many other side menus with links that need to open in new tabs then?
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.
Also, that link says it is not compatible with IE, I presume we are okay with that?
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 - could we also add noreferrer to handle IE (I think that's supported on newer IE versions)? 🙏
Re side menus and new tabs - we're not expecting to have any other external links. If it ever changes, we can rethink whether to commonise them :)
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.
Looks good! 👍
Ran all unit tests and tested manually.