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

Integration script #40

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Integration script #40

wants to merge 2 commits into from

Conversation

Adan206
Copy link
Collaborator

@Adan206 Adan206 commented Nov 3, 2017

Solved the original error.

@@ -74,6 +74,7 @@
<h2>Heading</h2>
<p>Donec id elit non mi porta gravida at eget metus. Fusce dapibus, tellus ac cursus commodo, tortor mauris condimentum nibh, ut fermentum massa justo sit amet risus. Etiam porta sem malesuada magna mollis euismod. Donec sed odio dui. </p>
<p><a class="btn btn-default" href="#" role="button">View details &raquo;</a></p>
<!-- Fixed Image Gallery -->
Copy link
Collaborator

@bvasilop bvasilop Nov 4, 2017

Choose a reason for hiding this comment

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

Consider adding a toggle state to the button for adding individual images. Also adding an aria value to assist with identifying the toggle button behavior.
<p><a class="btn btn-default" href="#" role="button" data-toggle="button" aria-pressed="false" ">View details &raquo;</a></p>

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed with @bvasilop's suggestion. Adding the aria-pressed will additionally meet accessibility criteria, making it easier for users with visual impairments to know what they're interacting with. Additionally, taking advantage of the aria-label will ensure a clear description of the button. Once the above changes are in place, resubmit PR for review and approval.

Copy link
Collaborator

@Cass0115 Cass0115 left a comment

Choose a reason for hiding this comment

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

Great update on fixing the bug.

Copy link
Collaborator

@georginajuarez georginajuarez left a comment

Choose a reason for hiding this comment

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

This would be in line with Integration and Social Media User Stories.

@@ -74,6 +74,7 @@
<h2>Heading</h2>
<p>Donec id elit non mi porta gravida at eget metus. Fusce dapibus, tellus ac cursus commodo, tortor mauris condimentum nibh, ut fermentum massa justo sit amet risus. Etiam porta sem malesuada magna mollis euismod. Donec sed odio dui. </p>
<p><a class="btn btn-default" href="#" role="button">View details &raquo;</a></p>
<!-- Fixed Image Gallery -->
<i class="fa fa-bookmark" aria-hidden="true"></i> <!-- Allows user to favorite poem -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

In addition to bookmarking the poem we should also add social icons so that the use may share the poem via social media: Facebook, Twitter, and Pinterest.

@@ -97,6 +98,8 @@
<script src="js/main.js"></script>

<!-- Google Analytics: change UA-XXXXX-X to be your site's ID. -->

<!-- Fixed The script, removed the original bug -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider moving google analytics script from <body> to <head> with adding <script async src='https://www.google-analytics.com/analytics.js'></script> for optimizing load times.

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

Successfully merging this pull request may close these issues.

None yet

6 participants