-
Notifications
You must be signed in to change notification settings - Fork 97
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
#387 Links Section is added #388
base: main
Are you sure you want to change the base?
Conversation
Hello @LaxRaj, thanks for raising a pull request in this project. The maintainers of this project are volunteers so please be understanding if it takes time before you get a response. We still appreciate your help with creating pull requests! |
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.
Hey Lakshya 👋 great start with the PR. I see you haven't used any formatter with the changes. I will suggest you to use Prettier Extension (on your preferred editor) and just format LinksTemplate.tsx
and push the changes here.
@@ -169,6 +169,38 @@ other places on the web */`} | |||
</li> | |||
</ul> | |||
</TemplateSection> | |||
{/* For issue #387 */} |
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.
I don't think this comment is necessary
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.
Alright removed that! and formatted the LinksTemplate.tsx
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.
The purpose of this website is to inform people about accessibility issues and give them examples of how to code in an accessible way.
I know there was no information in the issue (my fault, I was intending to write this section myself and created the issue as a reminder to myself) but I was thinking that this section would discuss the accessibility issues with opening links in new tabs/windows such as this not being the default behaviour of links so expectation is that they open in the same tab/window. By hijacking that expectation it can cause disorientation for people with cognitive problems or screen reader users that can't see the screen etc. It also removes choice for people as it is always possible to right click and open the link in a new window if you choose to but by adding the target removes the choice for those who want it to open in the same tab/window.
However, sometimes you don't have a choice (be that a stubborn designer or PO or some other constraints) so there should be examples of what to do If you must open links in a new tab/window eg. the pros/cons of adding an icon at the end of the link text or (opens in new window) either visibly or hidden.
Also, please remove the yarn.lock file from your PR.
Requested changes are updated. So far it's good. let me know if there is anything else you want me to work on or change. |
Added the Links Section
Link to issue
Closes #387
Checklist before requesting a review
and it shows no errors.
Additional Information (Optional)
The data I put in was not thorough as the web-app, So there might be a consistency issue. But it is up to date and defines accessibility issues clearly while using all those target attributes.