search

ranbuch / accessibility

add accessibility to your website
MIT License
287 stars 35 forks source link

Issue #24 Correct display in Safari #27

Closed infograf768 closed 2 years ago

infograf768 commented 2 years ago

https://github.com/ranbuch/accessibility/issues/24

Missing + in the js

brianteeman commented 2 years ago
  1. Surely this change should be made in the src not the dist?
  2. Are you sure that it should be font-family: Material+Icons? All the google documentation says not. I think you may be confused with the fontFaceSrc
infograf768 commented 2 years ago

@ranbuch As I was not much aware about this repo, I may have incorrectly patched the minified js instead of the main.js So the correction there would be line 26 fontFamily: 'Material Icons', to fontFamily: 'Material+Icons',

:)

infograf768 commented 2 years ago

Are you sure that it should be font-family: Material+Icons?

Yes. Try it for yourself. The + is taken off in the resulting css. Safari screen capture

Screenshot 2021-12-16 at 09 49 11 Screenshot 2021-12-16 at 10 06 09
brianteeman commented 2 years ago

I say again font-family should have a space in it. It is fontFaceSrc that should use the +

read the docs https://developer.mozilla.org/en-US/docs/Web/CSS/font-family

infograf768 commented 2 years ago

Read https://github.com/joomla/joomla-cms/issues/36311#issuecomment-995641561 about the redundant font-family.

Safari is real picky. The + is necessary or the the redundant css font-family has to be deleted from the js.

ranbuch commented 2 years ago

@infograf768 many thanks for your effort. For some reason I can see the change only on the dist/accessibility.min.js but not on src/main.js. Can you please add the + sign there as well?

If not, I can do it myself. Anyways, nice catch. Thanks again.

angieradtke commented 2 years ago

Hi guys - maybe offtopic here: but can somebody explain me why the openicon is an <i> instead of a button?

infograf768 commented 2 years ago

@ranbuch main.js also modified now.

ranbuch commented 2 years ago

Thanks @infograf768 merged. @angieradtke Yes, we should use button instead if li for better accessibility. Let's keep this issue open for now so I won't forget and implement it when I'll find the time to do so. Thanks all!

angieradtke commented 2 years ago

https://github.com/joomla/joomla-cms/issues/36311#issuecomment-996043196 https://github.com/angieradtke/Accessibility-plugin-icons/blob/main/README.md