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

UI: Update <a> tags to LinkTo's #17866

Merged
merged 5 commits into from
Nov 11, 2022
Merged

UI: Update <a> tags to LinkTo's #17866

merged 5 commits into from
Nov 11, 2022

Conversation

hashishaw
Copy link
Contributor

@hashishaw hashishaw commented Nov 9, 2022

<a> tags have different behavior than <LinkTo> within our app with regards to keeping root users logged in on page load. This change is to make sure we're using <a> tags as minimally as possible. All the current implementations of these tags are legitimate (they correctly reload the page or link to an external site) but to consolidate their usage I created a new ExternalLink component, and glimmerized DocLink and LearnLink so they can extend this new component.

Something I considered that is out of scope of this ticket, is using the HDS Links instead, but I think that should be follow-on work

@hashishaw hashishaw marked this pull request as ready for review November 10, 2022 15:11
@hashishaw hashishaw added the ui label Nov 10, 2022
@hashishaw hashishaw added this to the 1.13.0-rc1 milestone Nov 10, 2022
@hashishaw hashishaw changed the title Update <a> tags to LinkTo's UI: Update <a> tags to LinkTo's Nov 10, 2022
Copy link
Contributor

@zofskeez zofskeez left a comment

Choose a reason for hiding this comment

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

I really like these changes thanks for tackling! I left one more non-blocking comment.

Comment on lines +18 to +20
get href() {
return this.args.href;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this arg be accessed directly in the template instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one we want to keep, because the DocLink and LearnLink extend this component, and their hrefs are always a getter not a passed param

@hashishaw hashishaw merged commit 3c25033 into main Nov 11, 2022
@hashishaw hashishaw deleted the VAULT-9610/update-a-tags branch May 6, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants