Skip to content

Conversation

@mik-laj
Copy link

@mik-laj mik-laj commented Jul 26, 2016

Hello
Icons should not be read by screen readers

Read more at: http://fontawesome.io/accessibility/

I am managing the project for which the availability of websites is very important, and so this patch is important to me.
https://github.com/watchdogpolska/petycja-php.siecobywatelska.pl/commits/master

Thanks in advance

mik-laj pushed a commit to watchdogpolska/petycja-php.siecobywatelska.pl that referenced this pull request Jul 26, 2016
@Holt59
Copy link
Collaborator

Holt59 commented Jul 27, 2016

Thanks for the pull-request, it should be as you say but if I merge this commit it will break Travis due to the current tests for icons at BootstrapHtmlHelperTest.php#L49.

Would you be able to update these tests?


Also, a better approach for setting the aria-hidden option would be:

$options += [
    'aria-hidden' => true
];

This would allow the user to override this if he wants.

@mik-laj
Copy link
Author

mik-laj commented Jul 28, 2016

Done :-)

@codecov-io
Copy link

Current coverage is 49.28% (diff: 100%)

Merging #86 into master will increase coverage by 0.11%

@@             master        #86   diff @@
==========================================
  Files             9          9          
  Lines           901        903     +2   
  Methods          86         86          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            443        445     +2   
  Misses          458        458          
  Partials          0          0          

Powered by Codecov. Last update 0fc33eb...1d8f018

@Holt59 Holt59 merged commit f7988c1 into CakePHP-Bootstrap:master Jul 28, 2016
@Holt59
Copy link
Collaborator

Holt59 commented Jul 28, 2016

Thanks, I have merged the pull-request.

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.

3 participants