Skip to content
This repository has been archived by the owner on Jun 23, 2021. It is now read-only.

fixed minor accessibility issues #69

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sakshigupta265
Copy link

@sakshigupta265 sakshigupta265 commented Feb 4, 2021

Changes

The changes are made to fix issue #62
The website had minor accessibility issues like:

  • Form elements didn't have associated labels, added label for Email Address
  • Added title to Resume which will assist the user to Download Resume as a PDF

How Has This Been Tested?

The proposed changes were tested on the local computer, and a performance was checked using chrome lighthouse

Mentions

@Korusuke, kindly review the changes

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature and improvements (non-breaking change which adds/improves functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Code Refactoring (internal change which is not user facing)
  • Documentation
  • Test, CI, or build

Checklist

  • Coding style of this project is followed
  • Change in documentation is required
  • Documentation has been updated accordingly
  • Unit tests have been added to cover code change

@@ -3,7 +3,7 @@
<a href="{{ '/' | relative_url }}">
<div class="nav-logo">
<img src="./assets/img/logo.svg" />
<a class="nav-link" href="{{ site.url }}/cv/resume.pdf">Resume</a>
<a class="nav-link" href="{{ site.url }}/cv/resume.pdf" title="Download Resume as PDF">Resume</a>

Choose a reason for hiding this comment

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

This where the problem is coming from

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for helping!

Copy link
Member

@jkmartindale jkmartindale left a comment

Choose a reason for hiding this comment

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

Very small fix I'd like to see that isn't your fault, but will need to be fixed here since you're editing the line already:

{{ site.url }} isn't the right way to make absolute URLs in Jekyll. It works some of the time but can break. The proper way is to use {{ '/cv/resume.pdf' | absolute_url }}, which will work in all cases.

_includes/navbar.html Outdated Show resolved Hide resolved
_includes/navbar.html Outdated Show resolved Hide resolved
sakshigupta265 and others added 2 commits February 5, 2021 11:31
Co-authored-by: James Martindale <[email protected]>
Co-authored-by: James Martindale <[email protected]>
@sakshigupta265
Copy link
Author

Very small fix I'd like to see that isn't your fault, but will need to be fixed here since you're editing the line already:

{{ site.url }} isn't the right way to make absolute URLs in Jekyll. It works some of the time but can break. The proper way is to use {{ '/cv/resume.pdf' | absolute_url }}, which will work in all cases.

Thanks for the updates

@sakshigupta265
Copy link
Author

@Korusuke, please review it. Thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants